docker
Add a Shutdown Method to sdk.Handler
... by shutting down the http.Server using the proper method.
This is a WIP and I am very much open to suggestions. We might think about exposing the http.Server somehow so a plugin author can handle shutting it down.
This refers to docker/go-plugins-helpers#123
A library should not setup signal handlers. This should be done by implementing a Close method on Handler.
Right, something like that was my first thought but I tried to fix the behaviour I was seeing without modifying the API. How about adding Serve{TCP,Unix,Windows}Async functions then?
I am going to see what both options look like.
So. I changed the Handler#Serve method to run http.Server#Serve in a go routine. It returns a Closer implementation which shuts down the http.Server using its Shutdown method. The serve{TCP,Unix,Windows} methods wrap the Closer returned from Serve to additionally delete the spec file.
The resulting Closer is returned to the user to enable closing the Server in response to, e.g., OS signals.
There is a lot more work to be done, many other parts of the repository use the functions that I modified. I am still undecided whether I should change the return type or add new functions for async listening. The latter would not break the existing callsites obviously.
So, I will say, you are passing in the listener into the Serve call already. The caller can just close the listener... however this does not deal with active connections... for that you'd need to call Shutdown on the http server.
I think it may be best, in terms of adding a proper shutdown, to add a Shutdown(context.Context) to the Handler... but this will require storing the http.Server, and probably making everything use pointers.
These Serve<Proto> calls are not great... I recommend using just Serve and pass in a listener that you create.
I honestly don't know why we did anything in this library other than generate an http.ServeMux, though.
I followed your advice and added a Shutdown method to the sdk.Handler. Seems a lot cleaner though the user has to write more code now. I am certainly not a Go expert but the test suite passes and the volume plugin I am writing works fine. What do you think? Anything I can improve?
I'm hitting the same problem of not being able to cleanly terminate driver handlers ... which is especially bad for especially end-to-end testing.
Is there anything why this PR should not be moved forward again? Or would there be a chance for a new PR in case we can't breathe life into this one again, with the following design: turn sdk.Handler into a struct type that stores the current http.server (concurrency safe) and implements new Close and Shutdown pointer receivers?
