session.go: AcceptStream handles GoAway #18
Conversation
|
@ginkoob So I now remember why it works this way. The |
|
Ok but why does it work only from server to client? Why the client can't say "GoAway" to the server? I think this PR does just that. |
session.go
Outdated
| @@ -92,6 +95,7 @@ func newSession(config *Config, conn io.ReadWriteCloser, client bool) *Session { | |||
| streams: make(map[uint32]*Stream), | |||
| synCh: make(chan struct{}, config.AcceptBacklog), | |||
| acceptCh: make(chan *Stream, config.AcceptBacklog), | |||
| goAwayCh: make(chan struct{}, 1), | |||
There was a problem hiding this comment.
Little bit of a formatting fail (needs to be indented). Also, make(chan struct{}) should be sufficient.
There was a problem hiding this comment.
I think we should create the goAwayCh only if the peer is a server so I've moved it to the if client {}else{}. Let me know if you think otherwise.
I've noticed that now session_test.go:375 - client2.GoAway() always throws err: session shutdown. It shouldn't be an issue as the client shouldn't try to open connections after GoAway but still I don't know why it errors out.
|
Noted a couple small things. I see the distinction here - the go away won't cause the server blocked in |
|
Latest code looks good. Only last thing is the fail you mentioned in #18 (comment) - I can't reproduce that running your branch locally, and it doesn't make sense given how you've got the delay in there. Are you still seeing that? |
|
Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement Learn more about why HashiCorp requires a CLA and what the CLA includes root seems not to be a GitHub user. Have you signed the CLA already but the status is still pending? Recheck it. |
|
Given the age, I'm going to go ahead and close this PR. Per @armon's mention, GoAway wasn't meant to be sent from a client to a server anyway. |
|
@ginkoob If you can sign the CLA, I think we can merge this. |
Closes #17