-
Notifications
You must be signed in to change notification settings - Fork 318
Add a synchronous signal handler for graceful shutdown #1243
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
karlseguin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't seem to work on MacOS.
> zig build run
INFO app : server running . . . . . . . . . . . . . . . . . [+0ms]
address = 127.0.0.1:9222
^Cinfo: Received termination signal...
> zig build run
FATAL app : server run error . . . . . . . . . . . . . . . . [+0ms]
err = AddressInUse
FATAL app : exit . . . . . . . . . . . . . . . . . . . . . . [+0ms]
err = AddressInUse
I have to kill it a couple times before it's gone.
src/sighandler.zig
Outdated
| const assert = std.debug.assert; | ||
| const Allocator = std.mem.Allocator; | ||
|
|
||
| const Self = @This(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use Self for normal struct, only for generics.
src/sighandler.zig
Outdated
| assert(@typeInfo(@TypeOf(func)).@"fn".return_type.? == void); | ||
|
|
||
| const Args = @TypeOf(args); | ||
| const TypeErased = struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fancy, and it's fine. But it would also be ok to have a shutdown function in App/Server/whatever that takes an *anyopaque and knows to cast it back to its own type. I guess you wouldn't need an allocation anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This trick is only needed for type checking. You should copy the pointer to anyopaque anyway, so you won't waste any extra memory with .{server}. Since memory is allocated in the arena, the allocation here is almost equivalent to a pointer next to a pointer to a function.
237b05d to
6159d73
Compare
|
Does it work on macos? I tested it on my linux and in docker, it looks good to me in these cases. |
|
@krichprollsch, sorry, I didn't have MacOS at hand, I'll set it up this evening and check. |
|
np, we can also wait for a test from @karlseguin or @nikneym |
|
Just tested on MacOS, seems I have to run |
|
Interesting, this is actually a problem on macos, it doesn't interrupt accept after calling posix.shutdown, it needs an explicit posix.close. |
f41845e to
f7c5921
Compare
f7c5921 to
74eee75
Compare

Signals in Unix can be asynchronous (interrupting one of the threads and calling the
sighandlecallback) or synchronous (blocking the process on the sigwait call).Asynchronous signals seem easier to use, but they have a problem: since the system switches to sigaction at an arbitrary moment, the process can be stopped at an arbitrary point (for example, while acquiring a mutex or writing to a global buffer). Therefore, posix defines a specific list of functions that are safe to use in a handler. Using any other code (even the simplest ones like print) results in UB.
The code below circumvents this problem using blocking signal handling:
Since the thread is blocked on sigwait, it consumes no resources (except a few MB for its stack). If the program terminates, the thread will be killed when the main thread terminates.
Using atomic to get shutdown is necessary regardless of the thread I create here, posix states that the signal sent to the process can be handled on any of the threads.