Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The server should handle GoAway frames #17

Open
ginkoob opened this issue Sep 2, 2015 · 6 comments · May be fixed by #18
Open

The server should handle GoAway frames #17

ginkoob opened this issue Sep 2, 2015 · 6 comments · May be fixed by #18

Comments

@ginkoob
Copy link

ginkoob commented Sep 2, 2015

Currently GoAway is handled only on Session.Open. However there is also the case when the client sends a GoAway request. In that case the server should handle it on Accept(). A common use case is when the the client acts as a "server" (also known as back connection/ reverse connection)

@armon
Copy link
Member

armon commented Sep 2, 2015

I think we do still handle it. The recv() loop should cause the session to shutdown I believe

@ginkoob
Copy link
Author

ginkoob commented Sep 2, 2015

I think that's wrong though, isn't it? It should error ErrRemoteGoAway. Here is a test of the expected behavior(currently failing/deadlock):

func TestGoAwayClient(t *testing.T) {
client, server := testClientServer()
defer client.Close()
defer server.Close()
go func(){
if err := client.GoAway(); err != nil {
t.Fatalf("err: %v", err)
}
}()
_, err := server.Accept()
if err != ErrRemoteGoAway {
t.Fatalf("err: %v", err)
}
}

@ginkoob ginkoob linked a pull request Sep 2, 2015 that will close this issue
@ginkoob
Copy link
Author

ginkoob commented Sep 3, 2015

@armon I've submitted a PR. It would be great if you could have a look. It works but sometimes client2.GoAway()[0] fails (with shutdown error) for reasons I don't know though I don't think it's due the PR changes either.

[0] https://github.com/hashicorp/yamux/pull/18/files#diff-7f24d50c1e949c023a423eec758d017bR374

@marco-m
Copy link

marco-m commented Feb 7, 2019

@armon is this ticket still valid ?

@armon
Copy link
Member

armon commented Feb 11, 2019

@marco-m I'm not sure. I'm not close enough anymore. Would require somebody to spend some time digging into this.

@marco-m
Copy link

marco-m commented Feb 11, 2019

@armon thanks for answering!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants