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

Catch LinkageError during DynamicScope generation. #4285

Closed
wants to merge 1 commit into from

Conversation

@snowp
Copy link
Contributor

@snowp snowp commented Nov 14, 2016

When multiple threads are generating the same DynamicScope class, both
threads may try to generate the same class at the same time. When the second
thread hits defineClass, it throws a LinkageError because the class has already
been defined.

With this change we catch this exception and call loadClass to get the class that
the previous thread created.

When multiple threads are generating the same DynamicScope class, both
threads may try to generate the same class at the same time. When the second
thread hits defineClass, it throws a LinkageError becuase the class has already
been defined.

With this change we catch this exception and call loadClass to get the class that
the previous thread created.
@headius
Copy link
Member

@headius headius commented Nov 14, 2016

Maybe we should just lock around the class generation bit?

@headius headius added this to the JRuby 9.1.7.0 milestone Nov 14, 2016
@headius
Copy link
Member

@headius headius commented Nov 14, 2016

I will incorporate your patch but with some double-checked locking around class generation, so we can simply avoid the exception when there's parallel generation going on.

headius added a commit that referenced this pull request Nov 14, 2016
This is an enhancement to #4285 to avoid the exception altogether.

Any remaining exceptions that bubble out are intended to do so, so
we can see them and get reports and fix them. This modification
should effectively prevent double-loading (which caused the
LinkageError) but have reduced overhead.
@headius headius closed this Nov 14, 2016
@headius
Copy link
Member

@headius headius commented Nov 14, 2016

Great find, thanks!

@snowp
Copy link
Contributor Author

@snowp snowp commented Nov 14, 2016

Thanks for the fast response!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.