Skip to content
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

Try to fix deadlock in code loading #28416

Merged
merged 1 commit into from
Aug 3, 2018
Merged

Try to fix deadlock in code loading #28416

merged 1 commit into from
Aug 3, 2018

Conversation

Keno
Copy link
Member

@Keno Keno commented Aug 2, 2018

If a precompiled package's init function tries to itself require that
package (e.g. by importing a symbol from a submodule of that package),
we would run into a deadlock. Fix that by delaying the running of
initializers until after we've had a chance to register any root module.

Fixes #26028

If a precompiled package's init function tries to itself require that
package (e.g. by importing a symbol from a submodule of that package),
we would run into a deadlock. Fix that by delaying the running of
initializers until after we've had a chance to register any root module.

Fixes #26028
@Keno Keno merged commit 0e32871 into master Aug 3, 2018
@martinholters martinholters deleted the kf/codeloaddead branch August 3, 2018 04:59
@iblislin
Copy link
Member

iblislin commented Aug 3, 2018

FreeBSD CI workers got randomly frozen builds after this PR merged.

All the following builds are on 0e32871

@ararslan
Copy link
Member

ararslan commented Aug 3, 2018

I'm hesitant to backport this if it causes FreeBSD issues.

@Keno
Copy link
Member Author

Keno commented Aug 3, 2018

@iblis17 I'm not sure this is related. Those builds ran at about the same time and both hung at a place where they would be trying to do network I/O. The builds afterwards failed with a network error and the ones after that succeeded. Could there have been intermittent network problems?

@iblislin
Copy link
Member

iblislin commented Aug 4, 2018

Seems no hanging now. It may be network issue.

(But there aren't any exceptions raised. What's the timeout config of our libgit2.

ararslan pushed a commit that referenced this pull request Aug 4, 2018
If a precompiled package's init function tries to itself require that
package (e.g. by importing a symbol from a submodule of that package),
we would run into a deadlock. Fix that by delaying the running of
initializers until after we've had a chance to register any root module.

Fixes #26028

(cherry picked from commit 0e32871)
@@ -611,7 +611,8 @@ end
# or an Exception that describes why it couldn't be loaded
# and it reconnects the Base.Docs.META
function _include_from_serialized(path::String, depmods::Vector{Any})
restored = ccall(:jl_restore_incremental, Any, (Cstring, Any), path, depmods)
sv = ccall(:jl_restore_incremental, Any, (Cstring, Any), path, depmods)
restored = sv[1]
Copy link
Member

@KristofferC KristofferC Aug 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Keno, this looks incorrect. The return value from jl_restore_incremental is not indexable in case the return value is an exception?

Copy link
Member Author

@Keno Keno Aug 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This same commit changed the regular return value to be a tuple, but you are correct that something got messed up with the other return path in this function. Wanna fix it and ideally at a test case ;)?

KristofferC pushed a commit that referenced this pull request Feb 11, 2019
If a precompiled package's init function tries to itself require that
package (e.g. by importing a symbol from a submodule of that package),
we would run into a deadlock. Fix that by delaying the running of
initializers until after we've had a chance to register any root module.

Fixes #26028
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants