-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Not all binding implementations follow the requirements of fdb_stop_network #3015
Comments
It seems I may have been mistaken about the Java case, where it looks like we do block on the run network call terminating in our implementation of stop network. |
In Go land, it seems a bit harder to do. Ruby, Python and Java are using the API provided by language
So it seems like Go really emphasizes on explicitly handling cleanup up stuff, and the solution has to be change in API itself. |
It could be that the real requirement is that you join the network thread before returning from main, and atexit is too late to avoid the undefined behavior (atexit is also when global destructors are run) |
Or maybe the fact that we are waiting for |
@vishesh So are you saying that we should expose the |
Indeed it is. This is the best I could come up with: //go:linkname runtime_addExitHook runtime.addExitHook
func runtime_addExitHook(f func(), runOnNonZeroExit bool)
func init() {
// this is a mitigation for https://github.com/apple/foundationdb/issues/3015
// and it has the purpose of having our tests with -race enabled not crash with SIGSEGV
// due to the destructors being invoked while the network thread is still running
runtime_addExitHook(fdb.StopNetwork, true)
} I am exposing |
In the current C API, it is required that
fdb_stop_network
be called and the network thread allowed to complete before terminating the program. It seems we aren't doing this in all of our bindings, and absent a change to this requirement as proposed in #2978, this can lead to undefined behavior.It looks like the current state of our in-tree bindings are that:
If #2978 is done, then this won't be an issue. However, I suspect that solving this problem is easier than the other one, so it may be worth updating the two bindings in the meantime.
The text was updated successfully, but these errors were encountered: