Rework Server.Shutdown semantics. #484
Conversation
zenhack
commented
Mar 21, 2023
zenhack
commented
- Don't block on the shutdown hook actually being run. Relying on this behavior was dubious anyway, since it couldn't be relied upon for remote references, only Servers.
- Don't cancel existing calls when the last reference to the server is dropped. Callers can cancel the contexts themselves if that's what they want. Fixes Dropping the last reference to a Server should not cancel outstanding calls #423.
- Don't block on the shutdown hook actually being run. Relying on this behavior was dubious anyway, since it couldn't be relied upon for remote references, only Servers. - Don't cancel existing calls when the last reference to the server is dropped. Callers can cancel the contexts themselves if that's what they want. Fixes capnproto#423.
| default: | ||
| t.Error("Bootstrap client still alive after Close returned") | ||
| } | ||
| <-srvShutdown // Hangs if bootstrap client is never shut down. |
There was a problem hiding this comment.
Do we maybe want to do a select against some timer? Or use assert.Eventually(...)?
I generally dislike tests that can hang.
There was a problem hiding this comment.
Eh, we've ran into problems with spurious failures due to timeouts being too short often enough before that I'm hesitant to do so. We've actually fixed tests by removing the timeouts a handful of times.
There was a problem hiding this comment.
We could also crank up the timeouts to something like 5s. If it takes that long, it seems like that's in and of itself a bug, no?
Totally your call, though. Not a huge deal either way.
There was a problem hiding this comment.
I'm going to merge without. I feel like when we've hit problems before it may have been a matter of GitHub doing something weird; you're right that in a lot of these cases multiple seconds should be plenty, but we've seen it...
2 participants
