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

Global variables read stale values #4797

Closed
eregon opened this Issue Sep 25, 2017 · 3 comments

Comments

Projects
None yet
2 participants
@eregon
Member

eregon commented Sep 25, 2017

Environment

  • JRuby 9.1.13.0 and before
  • Linux

Global variable reads should behave as if they are volatile, so they always get the latest value.

However, it seems the invokedynamic implementation is not thread-safe or make some of the reads just get stale values, and never see the new value.

See bug_jruby_globals.rb to reproduce.

It accepts a number of threads as an argument.
Locally, the script never terminates with 4 threads, it does 1 to ~5 iterations and gets stuck.

Passing -Xinvokedynamic.global.maxfail=0 solves the problem so it seems related to invokedynamic globals. Running on MRI or TruffleRuby (which also treat global variables as constants until they are changed) also works fine.

@eregon

This comment has been minimized.

Show comment
Hide comment
@eregon

eregon Sep 25, 2017

Member

There seems to be other thread safety issues in the implementation of globals:

  • Creating globals is racy:
    private GlobalVariable createIfNotDefined(String name) {
    GlobalVariable variable = globalVariables.get(name);
    if (variable == null) {
    variable = GlobalVariable.newUndefined(runtime, name);
    globalVariables.put(name, variable);
    }
    return variable;
    }
  • UndefinedAccessor just replaces the GlobalVariable's accessor field, which is not volatile, which could lead to reading a nil (if the invalidator is not checked such as on the slow path) when the variable was already set.
Member

eregon commented Sep 25, 2017

There seems to be other thread safety issues in the implementation of globals:

  • Creating globals is racy:
    private GlobalVariable createIfNotDefined(String name) {
    GlobalVariable variable = globalVariables.get(name);
    if (variable == null) {
    variable = GlobalVariable.newUndefined(runtime, name);
    globalVariables.put(name, variable);
    }
    return variable;
    }
  • UndefinedAccessor just replaces the GlobalVariable's accessor field, which is not volatile, which could lead to reading a nil (if the invalidator is not checked such as on the slow path) when the variable was already set.
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Oct 4, 2017

Member

Nice find, thanks. I feel like the way our globals are structured needs a reboot (the current way is WAY over-abstracted) but I'll have a look at fixing this in place for now.

Member

headius commented Oct 4, 2017

Nice find, thanks. I feel like the way our globals are structured needs a reboot (the current way is WAY over-abstracted) but I'll have a look at fixing this in place for now.

@headius headius added this to the JRuby 9.1.14.0 milestone Oct 9, 2017

headius added a commit that referenced this issue Oct 9, 2017

Temporarily disable global variable value caching in JIT.
There are numerous races involved in caching this value, so while
we work to replace the global var subsystem we should disable the
value cache.

See #4797.

headius added a commit that referenced this issue Oct 9, 2017

Temporarily disable global variable value caching in JIT.
There are numerous races involved in caching this value, so while
we work to replace the global var subsystem we should disable the
value cache.

See #4797.
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Oct 9, 2017

Member

A fix is in for 9.1.14 that basically just disables the caching like @eregon did. I am resolving this as fixed and will file a separate bug for the many race conditions we need to deal with.

Member

headius commented Oct 9, 2017

A fix is in for 9.1.14 that basically just disables the caching like @eregon did. I am resolving this as fixed and will file a separate bug for the many race conditions we need to deal with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment