Fix Shutdown()

[tc-hib]

This is to fix #24

• Add test in sse_test.go for Shutdown() panic and goroutine leak
• Avoid panic: don't close removeClient (I think it's useless, and it would require a sort of wait group)
• Avoid leak: don't block on removeClient<- when an HTTP Handler ends because of the client being removed already
  • To this end, I've added a buffered channel on client to signal the client has already been removed. It should be garbage collected if it is never read.

Maybe we could make Shutdown work like http.Server.Shutdown : return on completion, and take a context.Context for deadline.

[alexandrevicenzi]

The fix does not sound right, and the test is odd. See my comment in #24.

[tc-hib]

I don't understand why the test is odd.
In my app, when I call s.Shutdown while at least one client is waiting for events, I get a panic.

I would not get it if my app exited quickly enough to actually not shut down, like in other tests.

Calling defer s.Shutdown() in main(), while s.Shutdown() doesn't wait for completion, is almost a no-op indeed.
A Go app doesn't wait for any goroutine to complete before exiting, so s.Shutdown() hardly has any chance to do anything.

See http.Server.Shutdown() which blocks until either its completion or a context cancellation. I think this is how it should work in go-sse.

[tc-hib]

Here is a little more material, hopping it helps:

• In the examples, the call to s.Shutdown() will never happen, because one has to "kill" the program to exit.
  • Here is a simple example of an HTTP server with graceful shutdown.
• Channels are not meant to be closed unless you have a range loop. Here is a quote from Ian Lance Taylor, and a comment from the Go Tour.

Ian Lance Taylor

Perhaps it would help to consider that the close function was introduced
for one purpose: to permit range over a channel to work. Without a
general signaling mechanism to indicate that there is no more data,
there would be no way to range over a channel. If you need some other
communication approach, then you shouldn't be using close.
-- Ian Lance Taylor

Go Tour

Channels aren't like files; you don't usually need to close them. Closing is only necessary when the receiver must be told there are no more values coming, such as to terminate a range loop.
-- A Tour of Go

---

Finally, if you modify the examples a little, you can reproduce the bug using curl as a client:

• In net_http.go, for example, remove defer s.Shutdown() and replace the last line (call to http.ListenAndServe) with the code bellow
• run the test
• In another terminal run curl http://localhost:3000/events/channel-1
• Wait a little and it should panic

Code for net_http.go

go http.ListenAndServe(":3000", nil)
time.Sleep(5 * time.Second) // Use this time to run "curl http://localhost:3000/events/channel-1"
s.Shutdown()
time.Sleep(10 * time.Millisecond) // Should panic, now that s.Shutdown() has time to complete

---

In my test, I was trying to:

• Let the shut down actually happen
• Have the client part happen without running curl

Is it why it looked odd?

I hope it's all clearer now :) Thanks again.

[alexandrevicenzi]

You have found a very unusual bug, it only fails if the remote client closes the connection while the shutdown is in progress.

To explain a bit:

• Shutdown forces all see channels to close
• SSE Channel close forces all clients to be disconnected
• Shutdown closes all go channels
• Client disconnect is received by ServeHTTP because it's released
• ServeHTTP attempt to remove a client that does not exist anymore by sending a message to a closed channel because it was shutdown
• bang, panic

I managed to reproduce it and I managed to fix (I think) the proper way. The wrong part is in ServeHTTP function. I also have found that the Shutdown never really waited on anything to be shut down, also fixing that.

I'll be closing this PR because there's a better way to fix it, but this helped me understand the problem.

Thanks.

[tc-hib]

I've actually found and fixed several other issues, but found it a little difficult to move forward through PR, so decided to keep my fork and use it instead. You can have a look at it if you want to spot other minor mistakes or lacks.
Thanks again.

[alexandrevicenzi]

I took a look, lots of changes.

My changes will be a major refactoring, rewriting the server, I'll probably fix some issues and create new ones. I also have a plan to create on connect/disconnect functions.