fixed server and worker to be gracefully shut down

[sio4]

Fixed server and worker to be gracefully shutting down.

Since the current version of buffalo does not support graceful shutdown properly, this could be a serious security (integrity) issue if the application is interrupted while it has uncompleted request handling or background job running.

It seems like the graceful shutdown mechanism was broken while implementing multiple server support. (#1039)

This patch is tested with following scenarios:

• properly stop listening
• properly completes ongoing requests
• properly completes ongoing background job: a 5 seconds long background job running with a simple worker

fixes: #2198

[sio4]

Findings and fixes:

• 3fbd918 - there were two waiting points for context.Done() and it made an immediate exit on interrupt. To fix this issue, sync.WaitGroup is used for server and worker.
• 002824e - server.Shutdown() was called with already canceled context. The argument must not be a canceled context to make the shutdown function works properly.
• 8d8f521 - reorder shutdown process from worker then server to server then worker. It would be better if we block incoming requests by stopping the listener, while we still keep the worker for long-running tasks.
  • We need a shutdown timeout option here.
  • Fixed the location of event emitting points (especially for the meaning of "... was called")
  • Cleaned up log messages

I feel like we need to check some more about process control flow, request flow, and some more thing from those points.

[paganotoni]

This is great @sio4. Let me know when this is ready for review.

[sio4]

• 0a640e1 - replaced @markbates 's http://github.com/markbates/sigtx with the standard signal.NotifyContext() which was introduced since 1.16, the oldest supported version. :-)
• 61fb3fe - just added shutdown timeout option, TimeoutSecondShutdown which is used as a context timeout for Shutdown call. So all pending requests should be handled within this time duration (which is 60 seconds default), or developers should configure a reasonable value for their application. I guess with will need some more timeout values in the future.

[sio4]

This is great @sio4. Let me know when this is ready for review.

Hi! Thanks @paganotoni for your comment! The PR is now ready for review with essential fixes. Please take a look at the PR and let me know if something needs to be fixed or needs to be clear.

[paganotoni]

Looks good to me. @sio4 is there any way to add tests for this changes?. It would be good to specify how this should behave through the tests to protect the functionality as the package evolves.

[sio4]

Yeah, I am thinking the same. There was a broken thing from the start (calling Shutdown() with canceled Context) and also another issue (duplicated Done checking) made after we started to support multiple servers. If we had test cases, it could be caught earlier.

However, I am not sure which kind of test case would be better. For testing this correctly, a semi-complete application that has a long-running request handler and a long-running job handler is required. Also, it could be a time-consuming task since it will cover timing behavior. I am thinking if it is good to have a unit test in this package or if it is the time we need to consider an integrated test with separated testing scenarios.

I am considering the first approach for now and will start to write one soon. However, please let me know if you have a good idea of any kind.

[paganotoni]

@sio4 also, one thought I had was about the event kinds (variables) being moved moved within the worker package. That would be a breaking change, right? Could we keep the event variables in the same place?

[sio4]

Yeah, agree. That could be a breaking change since they are public. Actually, I am not sure why they are public and at the same time, it seems like some of my changes related to them are not in a good direction. I will check them and will fix them.

By the way, I wrote test cases for this issue but since this is a highly timing-related issue, making a perfect test case is not easy. I wrote unit tests for the server.go file and also created a separate test app for this issue. I will test and investigate some more and will update.

[sio4]

Please take a look at the change and test case.

[paganotoni]

Thanks @sio4. This is looking good.

[paganotoni]

we just need to solve the conflicts and we should be good to go.

[sio4]

Thanks!