8261921: ClassListParser::current should be used only by main thread #2619

iklam · 2021-02-18T02:25:38Z

During -Xshare:dump, ClassListParser::current() should be used only by the main thread, which has created the only ClassListParser instance. Accessing it from other threads could cause intermittent failures. We observed this only on certain hosts with -Xcomp.

The fix is to check for ClassListParser::is_parsing_thread() before calling ClassListParser::current(). After the fix, I can no longer reproduce the crash.

I also did some renaming and comment cleaning.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8261921: ClassListParser::current should be used only by main thread

Reviewers

David Holmes (@dholmes-ora - Reviewer) ⚠️ Review applies to 08dc542
Calvin Cheung (@calvinccheung - Reviewer) ⚠️ Review applies to 9aeb9c1
Coleen Phillimore (@coleenp - Reviewer) ⚠️ Review applies to 9aeb9c1

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2619/head:pull/2619
$ git checkout pull/2619

bridgekeeper · 2021-02-18T02:25:49Z

👋 Welcome back iklam! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2021-02-18T02:26:59Z

@iklam The following label will be automatically applied to this pull request:

hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2021-02-18T02:30:52Z

Webrevs

dholmes-ora

Hi Ioi,

Thanks for the side-bar explanations on the background here.

Changes seem fine, but a couple of comments below.

Thanks,
David

dholmes-ora · 2021-02-18T05:33:46Z

src/hotspot/share/classfile/classListParser.cpp

+  // _instance should only be accessed by the thread that created _instance.
+  assert(_instance == NULL, "must be singleton");
+  _instance = this;
+  Atomic::store(&_parsing_thread, Thread::current());


The use of atomics here seems overkill as we have no issues with atomicity, tearing or ordering, when checking if the field is the current thread. You can't get a false positive and the current thread must always see the value it set.

As we discussed off-line, theoretically we could have tearing if _parsing_thread spanned across a cache line. We don't actually support such platforms today (as you pointed out, Atomic:load is just a naked load for now). However, it's better to use the proper atomic operations for future proofing.

dholmes-ora · 2021-02-18T05:34:47Z

src/hotspot/share/classfile/classListParser.cpp

 ClassListParser::~ClassListParser() {
  if (_file) {
    fclose(_file);
  }
+  Atomic::store(&_parsing_thread, (Thread*)NULL);
  _instance = NULL;
 }


This gives the impression that we can create a new ClassListParser later, potentially in a different thread - is that really the case?

Currently we don't do that, but I want to make sure we have proper clean up. Since we are modifying _instance, we should clean up other statics as well.

dholmes-ora · 2021-02-18T05:35:29Z

src/hotspot/share/classfile/classListParser.hpp

@@ -81,6 +83,7 @@ class ClassListParser : public StackObj {
    _line_buf_size        = _max_allowed_line_len + _line_buf_extra
  };

+  static Thread* _parsing_thread;    // the thread that created _instance


I forget whether we still want to use volatile here or not.

_parsing_thread and _instance are read by another thread so that's why they have Atomic::load and store. This should be volatile if they need that level of memory ordering.

I've changed it to volatile.

dholmes-ora · 2021-02-18T05:37:10Z

src/hotspot/share/classfile/systemDictionaryShared.hpp

-                                                bool is_superclass,
-                                                TRAPS);
+  static InstanceKlass* lookup_super_for_unregistered_class(Symbol* class_name,
+                            Symbol* super_name,  bool is_superclass);


Indentation is ad-hoc here, please align Symbol

openjdk · 2021-02-18T05:39:11Z

@iklam This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8261921: ClassListParser::current should be used only by main thread

Reviewed-by: dholmes, ccheung, coleenp

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 2 new commits pushed to the master branch:

991f7c1: 8210373: Deadlock in libj2gss.so when loading "j2gss" and "net" libraries in parallel.
0217d69: 8261975: Missing "classpath exception" in VectorSupport.java

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

calvinccheung · 2021-02-19T01:14:45Z

src/hotspot/share/classfile/systemDictionary.cpp

+    InstanceKlass* k = SystemDictionaryShared::lookup_super_for_unregistered_class(class_name,
+                           super_name, is_superclass);


Please align super_name with class_name in the previous line.

calvinccheung · 2021-02-19T01:16:44Z

src/hotspot/share/classfile/classListParser.hpp

@@ -81,6 +83,7 @@ class ClassListParser : public StackObj {
    _line_buf_size        = _max_allowed_line_len + _line_buf_extra
  };

+  static Thread* _parsing_thread;    // the thread that created _instance


Should this be declared volatile?

calvinccheung

Looks good. Just couple of comments.

Thanks,
Calvin

coleenp

Looks good.

coleenp · 2021-02-19T02:38:38Z

src/hotspot/share/classfile/classListParser.hpp

@@ -81,6 +83,7 @@ class ClassListParser : public StackObj {
    _line_buf_size        = _max_allowed_line_len + _line_buf_extra
  };

+  static Thread* _parsing_thread;    // the thread that created _instance


_parsing_thread and _instance are read by another thread so that's why they have Atomic::load and store. This should be volatile if they need that level of memory ordering.

…classlistparser-only-main-thread

dholmes-ora

Still good.

Thanks,
David

dholmes-ora · 2021-02-23T00:45:03Z

src/hotspot/share/classfile/classListParser.cpp

@@ -81,8 +81,7 @@ ClassListParser::ClassListParser(const char* file) {
 }

 bool ClassListParser::is_parsing_thread() {
-  Thread* t = Atomic::load(&_parsing_thread);
-  return (t == Thread::current());
+  return (Atomic::load(&_parsing_thread) == Thread::current());


Nit: superfluous brackets on a binary operator

iklam · 2021-02-23T03:11:20Z

Thanks @calvinccheung @coleenp @dholmes-ora for the review!
/integrate

openjdk · 2021-02-23T03:12:11Z

@iklam Since your change was applied there have been 2 commits pushed to the master branch:

991f7c1: 8210373: Deadlock in libj2gss.so when loading "j2gss" and "net" libraries in parallel.
0217d69: 8261975: Missing "classpath exception" in VectorSupport.java

Your commit was automatically rebased without conflicts.

Pushed as commit 8cfea7c.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8261921: ClassListParser::current should be used only by main thread

9aeb9c1

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Feb 18, 2021

iklam marked this pull request as ready for review February 18, 2021 02:27

openjdk bot added the rfr Pull request is ready for review label Feb 18, 2021

dholmes-ora approved these changes Feb 18, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 18, 2021

calvinccheung reviewed Feb 19, 2021

View reviewed changes

calvinccheung approved these changes Feb 19, 2021

View reviewed changes

coleenp approved these changes Feb 19, 2021

View reviewed changes

ioilam added 2 commits February 22, 2021 15:52

changed _parsing_thread to volatile

eae4fa3

Merge branch 'master' of https://github.com/openjdk/jdk into 8261921-…

08dc542

…classlistparser-only-main-thread

dholmes-ora approved these changes Feb 23, 2021

View reviewed changes

removed unnecessary parenthesis

fd902f1

openjdk bot closed this Feb 23, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 23, 2021

8261921: ClassListParser::current should be used only by main thread #2619

8261921: ClassListParser::current should be used only by main thread #2619

iklam commented Feb 18, 2021 •

edited by openjdk bot

bridgekeeper bot commented Feb 18, 2021

openjdk bot commented Feb 18, 2021

mlbridge bot commented Feb 18, 2021 •

edited

dholmes-ora left a comment

dholmes-ora Feb 18, 2021

iklam Feb 22, 2021

dholmes-ora Feb 18, 2021

iklam Feb 22, 2021

dholmes-ora Feb 18, 2021

coleenp Feb 19, 2021

iklam Feb 22, 2021

dholmes-ora Feb 18, 2021

iklam Feb 22, 2021

openjdk bot commented Feb 18, 2021 •

edited

calvinccheung Feb 19, 2021

iklam Feb 22, 2021

calvinccheung Feb 19, 2021

iklam Feb 22, 2021

calvinccheung left a comment

coleenp left a comment

coleenp Feb 19, 2021

dholmes-ora left a comment

dholmes-ora Feb 23, 2021

iklam commented Feb 23, 2021

openjdk bot commented Feb 23, 2021

		InstanceKlass* k = SystemDictionaryShared::lookup_super_for_unregistered_class(class_name,
		super_name, is_superclass);

8261921: ClassListParser::current should be used only by main thread #2619

8261921: ClassListParser::current should be used only by main thread #2619

Conversation

iklam commented Feb 18, 2021 • edited by openjdk bot

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Feb 18, 2021

openjdk bot commented Feb 18, 2021

mlbridge bot commented Feb 18, 2021 • edited

Webrevs

dholmes-ora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Feb 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calvinccheung left a comment

Choose a reason for hiding this comment

coleenp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dholmes-ora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iklam commented Feb 23, 2021

openjdk bot commented Feb 23, 2021

iklam commented Feb 18, 2021 •

edited by openjdk bot

mlbridge bot commented Feb 18, 2021 •

edited

openjdk bot commented Feb 18, 2021 •

edited