Prevent fork from happening in known fork unsafe API #10864
Conversation
casperisfine
force-pushed
the
fork-lock
branch
3 times, most recently
from
cfd7fcd
to
058e40e
Compare
This comment has been minimized.
This comment has been minimized.
ext/socket/raddrinfo.c
Outdated
| @@ -498,9 +524,6 @@ rb_getaddrinfo(const char *hostp, const char *portp, const struct addrinfo *hint | |||
| errno = err; | |||
| return EAI_SYSTEM; | |||
| } | |||
| pthread_detach(th); | |||
|
|
|||
| rb_thread_call_without_gvl2(wait_getaddrinfo, arg, cancel_getaddrinfo, arg); | |||
There was a problem hiding this comment.
Was this deleted on purpose? it seems important to me! (it's the bit that actually waits on the condvar for the lookup to be done, right?)
There was a problem hiding this comment.
Nah, just a bad rebase I think, let me check.
There was a problem hiding this comment.
Ok, fixed it. It should have been replaced by fork_safe_async_getaddrinfo(arg);
ext/socket/raddrinfo.c
Outdated
| static void * | ||
| fork_safe_async_getaddrinfo(void *ptr) | ||
| { | ||
| return rb_thread_prevent_fork(async_getaddrinfo, ptr); |
There was a problem hiding this comment.
@akr pointed out that it is wrong to call rb_thread_prevent_fork here.
If Ruby's getaddrinfo is interrupted, such as by Ctrl+C, the pthread in question calling getaddrinfo(3) may remain even after async_getaddrinfo returns. So this does not satisfy your purpose (to avoid fork is called during getaddrinfo(3) execution).
Instead, you should guard the call of getaddrinfo(3) (and freeaddrinfo(3) too?), in do_getaddrinfo.
Note that rb_thread_prevent_fork would be called with no GVL in this case. Because rb_ensure requires GVL, rb_thread_prevent_fork must not use it. I think rb_thread_prevent_fork doesn't have to guard longjmp.
There was a problem hiding this comment.
Thank you @mame. I finally figured my linking error, and made changes based on your review.
I think it's much simpler now, how does it look?
There was a problem hiding this comment.
@casperisfine Thanks! It looks good to me. @akr Could you review it too?
There was a problem hiding this comment.
That's great. Does that means you are good with merging it, or do you wish to add it at a dev meeting first? https://bugs.ruby-lang.org/issues/20590
There was a problem hiding this comment.
🎉 I rebased it and enabled auto-merge. Thanks again!
casperisfine
force-pushed
the
fork-lock
branch
2 times, most recently
from
d784192
to
ad730ba
Compare
Head branch was pushed to by a user without write access
casperisfine
force-pushed
the
fork-lock
branch
from
ad730ba
to
1429639
Compare
…unsafe API [Feature #20590] For better of for worse, fork(2) remain the primary provider of parallelism in Ruby programs. Even though it's frowned uppon in many circles, and a lot of literature will simply state that only async-signal safe APIs are safe to use after `fork()`, in practice most APIs work well as long as you are careful about not forking while another thread is holding a pthread mutex. One of the APIs that is known cause fork safety issues is `getaddrinfo`. If you fork while another thread is inside `getaddrinfo`, a mutex may be left locked in the child, with no way to unlock it. I think we could reduce the impact of these problem by preventing in for the most notorious and common cases, by locking around `fork(2)` and known unsafe APIs with a read-write lock.
casperisfine
force-pushed
the
fork-lock
branch
from
1429639
to
c6169d6
Compare
casperisfine
changed the title
[Feature #20590]
For better of for worse, fork(2) remain the primary provider of parallelism in Ruby programs. Even though it's frowned uppon in many circles, and a lot of literature will simply state that only async-signal safe APIs are safe to use after
fork(), in practice most APIs work well as long as you are careful about not forking while another thread is holding a pthread mutex.One of the APIs that is known cause fork safety issues is
getaddrinfo. If you fork while another thread is insidegetaddrinfo, a mutex may be left locked in the child, with no way to unlock it.I think we could reduce the impact of these problem by preventing in for the most notorious and common cases, by locking around
fork(2)and known unsafe APIs with a read-write lock.FYI: @mame @beauraF