gracefully close connection fixes: #448

[sanketplus]

Creating PR to request and accommodate comments. There are multiple areas which requires decision making.

1. whether to maintain backwards compatibility with variadic args, or is there a better approach.
2. after sending a close frame, what to do exactly. Wait for sometime and give up? How long to wait?
3. Do we need to set kind of read lock for public read methods once close has been initiated?
4. How exactly do I interpret output of wstest?

<- suggestions

[ghost]

How about doing something like this instead:

// Shutdown sends a close message to the peer, waits for the peer to complete
// the closing handshake and closes the connection.  Shutdown assumes that the
// application is reading the connection in another goroutine.
func (c *Conn) Shutdown(closeCode int, closeMessage string, timeout time.Duration) error {
      // check for proper code and message
      message := FormatCloseMessage(closeCode, closeMessage)
      c.WriteControl(CloseMessage, message, time.Now().Add(writeWait))  // ignore error
      select {
      case <- time.After(timeout):
      case <- c.done:
      }
      return c.Close()
}

The done field is a channel that's closed on receipt of a close message.

[sanketplus]

Accommodated comments @stevenscott89
I was experimenting with intercepting readers when they finish reading (ie: at start of NextReader and when we break out of loop in Read) and hand control over to Shutdown from there if shutdown has initiated. But it again had two flaws.

1. If there was no reader in first place, we cannot detect shutdown and hand control over
2. handing control over does not do much because existing reader can read Close frame and inform shutdown.

So the question remains is, how do we detect if there is no reader and try to read close frame. And do we want to do this? RFC says after receiving CLOSE an endpoint shall close connection. But if we don't try to read and there is no reader also, we violate this and close conn only on timeout.

[ghost]

The new channel field should be grouped with other read related fields and should be closed when c.readErr transitions from nil to non-nil. A function should be added for setting c.readErr instead of sprinkling the logic throughout the code.

It's impossible to detect if the application is reading the connection or not. I think the application must specify this when calling shutdown. The API is getting cumbersome with yet another argument to shutdown.

[sanketplus]

I agree, approaches would be debatable. Do you think this can be merged now and can later be matured when we discover more suitable ways to get it closer to RFC?

[garyburd]

I left comments in #448. The PR is not ready for merge:

• To avoid breaking changes, the API must have the correct design from the get go. Specially, there must be a plan for handling shutdown from reading goroutine and shutdown from some other goroutine.
• I agree with @stevenscott89's comments regarding the channel.

This PR may need to wait for a new project maintainer to review.

[sanketplus]

thanks @garyburd for suggestions. I had some ideas implemented on my dev setup to detect a read end (similar to readDone that you mentioned). I'll try to have it added to this PR. Putting it on back burner for now as we do not see it getting merged soon.

[feliperyan]

Would this achieve a graceful shutdown? https://gist.github.com/feliperyan/2160d6df63f502079d7995372bc09e5c

[IngCr3at1on]

@feliperyan unless I'm crazy that gist will block in at least some cases because you are never receiving on the joining channel and it's not buffered.

[feliperyan]

@IngCr3at1on Joining channel receives in the manageConns func. What im not certain about is: let’s say there’s 1000 clients and as I start iterating through them all, closing connections more and more new clients keep joining? I might never get to zero! How can I stop accepting new connections then start closing the existing connections?

[ghost]

@feliperyan Yes, the program gracefully closes the connections. You may want to shutdown the server before waiting for the connections to close.

Your question is not on the topic of this PR. Please open a new issue if you want to continue discussion about your program.

[sanketplus]

its been some time. have we found new maintainer by any chance?

[nhooyr]

Bunch of discussion on a very similar feature request in my library. coder/websocket#103

You might find it useful.

[jaitaiwan]

I'm assigning myself to this PR, it may take some time to get familiar with the problem space and understand it to make a decision. If you feel that this PR is superseded by another or no longer relevant please reach out.

[ekovacs]

@jaitaiwan when can we expect this to be merged?

[jaitaiwan]

LGTM, however I'd like either @apoorvajagtap or @AlexVulaj to review before a final final merge. From what I understand of the problem space it's okay to merge but it's a complex issue so just making sure that merging is the right move.

[FZambia]

Please let's make sure concerns mentioned by Gary Burd in #487 (comment) and #448 (comment) are taken into account in this PR. @jaitaiwan do I understand correctly you investigated this before approving, right?

[jaitaiwan]

Yep I've seen those. So far as I can tell, there's two arguments here:

1. The RFC requires this shutdown procedure but it's implementation needs to be carefully thought out (which it seems this version is)
2. Don't add anything and make it a requirement of implementing libraries to perform the RFC shutdown process.

It doesn't look like this PR changes any existing API interfaces, but it does add more - of which I don't see any issues with supporting especially considering it's a part of the RFC spec.

I'll hold on this and await more feedback

[AlexVulaj]

Suggested change

	// Shutdown sends a close frame to the peer and waits for close frame in resopnse.
	// Shutdown sends a close frame to the peer and waits for close frame in response.

[AlexVulaj]

@jaitaiwan

Don't add anything and make it a requirement of implementing libraries to perform the RFC shutdown process.

I'm inclined to go with this option unless there's a strong reason not to, as this change seems a bit controversial.

[ghost]

This PR does not provide a way for the the read goroutine to gracefully execute the closing handshake.

There is a way implement graceful shutdown without the pitfalls described in my comments and others.

[ghost]

The wording is not strong enough here. The documentation should say "The application must not call Shutdown from the reading goroutine."