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

[MODULES-270] fixing problem where ClassLoader.definePackage() can th… #111

Merged
merged 2 commits into from Oct 7, 2016

Conversation

ropalka
Copy link
Contributor

@ropalka ropalka commented Oct 7, 2016

…row IllegalArgumentException on JDK9

pkg = super.definePackage(name, specTitle, specVersion, specVendor, implTitle, implVersion, implVendor, sealBase);
} catch (final IllegalArgumentException iae) {
pkg = super.getPackage(name);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

When does this occur? Is this something new in Java 9?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed that on JDK8 whole method ClassLoader.definePackage() was synchronized.
In JDK9 they're using ConcurrentHashMap.putIfAbsent() instead so it opens a window
for potential concurrent update of the underlying package map. I think this design change is the reason why we can see IllegalArgumentException in some tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any chance an IllegalArgumentException can be thrown without the package being defined in the superclass?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we might want to add an ``if (pkg == null) throw iae;" after that getPackage call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, reworking ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@dmlloyd dmlloyd merged commit 69929c7 into jboss-modules:master Oct 7, 2016
@ropalka ropalka deleted the MODULES-270 branch October 7, 2016 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants