RFR: 8231627: runtime/ErrorHandling/ThreadsListHandleInErrorHandlingTest.java fails because error occurred during printing all threads

[Patricio Chilano Mateo]

On Thu, 24 Dec 2020 17:33:21 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

> A small robustness fix in ThreadsSMRSupport::print_info_on() to reduce the
> likelihood of crashes during error reporting. Uses Threads_lock->try_lock()
> for safety and restricts some reporting to when the Threads_lock has been
> acquired (depends on JDK-8256383). Uses a ThreadsListHandle to make
> the current ThreadsList safe for reporting (depends on JDK-8258284). Also
> detects when the system ThreadsList (_java_thread_list) has changed and
> will warn that some of the reported info may now be stale.
> 
> Two existing tests have been updated to reflect the use of a ThreadsListHandle
> in ThreadsSMRSupport::print_info_on(). Mach5 Tier[1-6] testing has no regressions.

Hi Dan,

Looks good to me!

src/hotspot/share/runtime/threadSMR.cpp line 1130:

> 1128: 
> 1129:   if (_to_delete_list != NULL) {
> 1130:     if (has_Threads_lock) {

Shouldn't we check for Threads_lock->owned_by_self() instead? (case where thread already owns Threads_lock before calling print_info_on()).

src/hotspot/share/runtime/threadSMR.cpp line 1148:

> 1146:     }
> 1147:   }
> 1148:   if (!EnableThreadSMRStatistics) {

You could switch to "if(EnableThreadSMRStatistics)" instead and put this code at the end to avoid repetition. Also I think the comparison with _java_thread_list could be done unconditionally at the end since it's already racy anyways (even if the info was printed with the Threads_lock held it could have changed right after it's released and before returning).

-------------

Marked as reviewed by pchilanomate (Committer).

PR: https://git.openjdk.java.net/jdk/pull/1891