Fix signal handling #10412
Fix signal handling #10412
Conversation
|
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
ocis/pkg/runtime/service/service.go
Outdated
| @@ -515,7 +514,7 @@ func trap(s *Service, ctx context.Context) { | |||
| } | |||
| } | |||
| s.Log.Debug().Str("service", "runtime service").Msgf("terminating with signal: %v", s) | |||
| os.Exit(0) | |||
| //os.Exit(0) // this seems to cause an early exit that prevents services from shitting down properly | |||
|
|
@fschade This is far from what we need to do. But it will at least give all reva services 3sec to deregister themselves. However, there as still a problem with the natsjskv registry: we are using a TTL and nats will forget the service nodes. Unfortunately, it does not emit an event for the delete, causing the caches in our go micro clients to never forget the nodes. Currently nodes are only forgotten if the service properly deregisters itself (which this PR fixes). If a node is OOM killed ... we don't forget it. I'll create a subseqent PR to put the TTL into the registry values and ignore them if they timed out. |
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
butonic
force-pushed
the
fix-signal-handling
branch
from
48d9918 to
04632ad
Compare
| @@ -515,7 +515,8 @@ func trap(s *Service, ctx context.Context) { | |||
| } | |||
| } | |||
| s.Log.Debug().Str("service", "runtime service").Msgf("terminating with signal: %v", s) | |||
| os.Exit(0) | |||
| time.Sleep(3 * time.Second) // give the services time to deregister | |||
| os.Exit(0) // FIXME this cause an early exit that prevents services from shitting down properly | |||
There was a problem hiding this comment.
It is still here btw 😄 but we can keep it as a reminder.
There was a problem hiding this comment.
Yes, I was thinking of removing the comment, but it feels right like it is. reva has no way to properly handle a context done triggered shutdown. A runtime.Shutdown(ctx) function like the http server would be great and would allow us to properly shut down all services ... currently we have no way of notifying decomposedfs to flush data to disk ...
|
|
regarding the code duplication, I wonder if we can use generics to reduce the amount of boilerplate we have ... |
signal handling seems broken ... or context cancelation is not always picked up properly ... needs more work