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

8286830: ~HandshakeState should not touch oops #8795

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -502,6 +502,16 @@ bool HandshakeState::has_async_exception_operation(bool ThreadDeath_only) {
}
}

void HandshakeState::clean_async_exception_operation() {
while (has_async_exception_operation(/* ThreadDeath_only */ false)) {
MutexLocker ml(&_lock, Mutex::_no_safepoint_check_flag);
HandshakeOperation* op;
op = _queue.peek(async_exception_filter);
remove_op(op);
delete op;
}
}

bool HandshakeState::have_non_self_executable_operation() {
assert(_handshakee != Thread::current(), "Must not be called by self");
assert(_lock.owned_by_self(), "Lock must be held");
@@ -133,6 +133,7 @@ class HandshakeState {
bool has_operation() { return !_queue.is_empty(); }
bool has_operation(bool allow_suspend, bool check_async_exception);
bool has_async_exception_operation(bool ThreadDeath_only);
void clean_async_exception_operation();

bool operation_pending(HandshakeOperation* op);

@@ -1364,6 +1364,7 @@ static bool is_daemon(oop threadObj) {
// cleanup_failed_attach_current_thread as well.
void JavaThread::exit(bool destroy_vm, ExitType exit_type) {
assert(this == JavaThread::current(), "thread consistency check");
assert(!is_exiting(), "should not be exiting or terminated already");
Copy link
Member

@dcubed-ojdk dcubed-ojdk Jun 3, 2022

Choose a reason for hiding this comment

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

Hmmm... The old destroy_vm == true branch assert was:

assert(!is_terminated() && !is_exiting(), "must not be exiting");

and the new assert mesg has "or terminated", but the assert is only
checking !is_exiting() and doesn't not include !is_terminated().
So the old assert checked both conditions and only mumbled about
"exiting" and the new assert checks one condition and mumbles
about both conditions.

Copy link
Contributor Author

@pchilano pchilano Jun 3, 2022

Choose a reason for hiding this comment

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

is_exiting() also checks is_terminated() internally, so it returns true for the _thread_exiting, _thread_terminated and _vm_exited cases (false only for the _not_terminated case). Since the extra !is_terminated() check was redundant I removed it. Also since the assert will trigger for both the exiting and terminated cases I added both to the assert msg.


elapsedTimer _timer_exit_phase1;
elapsedTimer _timer_exit_phase2;
@@ -1425,18 +1426,23 @@ void JavaThread::exit(bool destroy_vm, ExitType exit_type) {
if (JvmtiExport::should_post_thread_life()) {
JvmtiExport::post_thread_end(this);
}

// The careful dance between thread suspension and exit is handled here.
// Since we are in thread_in_vm state and suspension is done with handshakes,
// we can just put in the exiting state and it will be correctly handled.
set_terminated(_thread_exiting);

ThreadService::current_thread_exiting(this, is_daemon(threadObj()));
} else {
assert(!is_terminated() && !is_exiting(), "must not be exiting");
// before_exit() has already posted JVMTI THREAD_END events
}

// Cleanup any pending async exception now since we cannot access oops after
// BarrierSet::barrier_set()->on_thread_detach() has been executed.
if (has_async_exception_condition()) {
handshake_state()->clean_async_exception_operation();
}

// The careful dance between thread suspension and exit is handled here.
// Since we are in thread_in_vm state and suspension is done with handshakes,
// we can just put in the exiting state and it will be correctly handled.
// Also, no more async exceptions will be added to the queue after this point.
set_terminated(_thread_exiting);
Copy link
Member

@dholmes-ora dholmes-ora May 20, 2022

Choose a reason for hiding this comment

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

I would have expected that cleaning up any async exceptions has to happen after we set the _thread_exiting state; otherwise what happens if another thread is trying to install an async exception on the current thread before we set it?

Copy link
Contributor Author

@pchilano pchilano May 20, 2022

Choose a reason for hiding this comment

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

The ordering of cleaning up the async exceptions in the queue and the write of _thread_exiting doesn't really matter as long as the state remains _thread_in_vm. We install async exceptions inside a synchronous handshake, which can only happen while the target is handshake/safepoint safe. Since moving to a safe state by the target and reading of the state by the handshaker uses acquire/release semantics, the handshaker will already see the _thread_exiting value inside the handshake.

ThreadService::current_thread_exiting(this, is_daemon(threadObj()));
Copy link
Member

@dcubed-ojdk dcubed-ojdk Jun 3, 2022

Choose a reason for hiding this comment

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

Previously, we didn't call ThreadService::current_thread_exiting() in the
destroy_vm == true branch. Is that going to result in some change in
behavior?

Copy link
Contributor Author

@pchilano pchilano Jun 3, 2022

Choose a reason for hiding this comment

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

That call decrements a counter used for some stats and for determining the size of the thread array in ThreadListEnumerator. But I don't see any issues on doing that since this thread is actually exiting.


if (log_is_enabled(Debug, os, thread, timer)) {
_timer_exit_phase1.stop();
_timer_exit_phase2.start();
@@ -1528,7 +1534,8 @@ void JavaThread::exit(bool destroy_vm, ExitType exit_type) {
}
#endif // INCLUDE_JVMCI

// Remove from list of active threads list, and notify VM thread if we are the last non-daemon thread
// Remove from list of active threads list, and notify VM thread if we are the last non-daemon thread.
// We call BarrierSet::barrier_set()->on_thread_detach() here so no touching of oops after this point.
Threads::remove(this, daemon);
Copy link
Member

@dcubed-ojdk dcubed-ojdk Jun 3, 2022

Choose a reason for hiding this comment

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

Thanks for adding the new comment line!


if (log_is_enabled(Debug, os, thread, timer)) {
@@ -1698,8 +1705,9 @@ void JavaThread::handle_async_exception(oop java_throwable) {
}

void JavaThread::install_async_exception(AsyncExceptionHandshake* aeh) {
// Do not throw asynchronous exceptions against the compiler thread.
if (!can_call_java()) {
// Do not throw asynchronous exceptions against the compiler thread
// or if the thread is already exiting.
if (!can_call_java() || is_exiting()) {
delete aeh;
return;
}
@@ -72,7 +72,30 @@ public static void main(String[] args) {
usage();
}
}
test(timeMax);

// Fire-up deamon that just creates new threads. This generates contention on
// Threads_lock while worker tries to exit, creating more places where target
// can be seen as handshake safe.
Thread threadCreator = new Thread() {
@Override
public void run() {
while (true) {
Thread dummyThread = new Thread(() -> {});
dummyThread.start();
try {
dummyThread.join();
} catch(InterruptedException ie) {
}
}
}
};
threadCreator.setDaemon(true);
threadCreator.start();
test(timeMax);
Copy link
Member

@dcubed-ojdk dcubed-ojdk Jun 3, 2022

Choose a reason for hiding this comment

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

Okay, but now the test is going to execute for twice timeMax which might be
a bit of a surprise... In particular the usage() mesg is now wrong.

Copy link
Member

@dcubed-ojdk dcubed-ojdk Jun 3, 2022

Choose a reason for hiding this comment

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

Also, the @bug line is not updated with this bug ID.

Copy link
Contributor Author

@pchilano pchilano Jun 3, 2022

Choose a reason for hiding this comment

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

Ok, we could fix this in a new bug.

}

public static void test(int timeMax) {
System.out.println("About to execute for " + timeMax + " seconds.");

long count = 0;
@@ -78,7 +78,30 @@ public static void main(String[] args) {
usage();
}
}
test(timeMax);

// Fire-up deamon that just creates new threads. This generates contention on
// Threads_lock while worker tries to exit, creating more places where target
// can be seen as handshake safe.
Thread threadCreator = new Thread() {
@Override
public void run() {
while (true) {
Thread dummyThread = new Thread(() -> {});
dummyThread.start();
try {
dummyThread.join();
} catch(InterruptedException ie) {
}
}
}
};
threadCreator.setDaemon(true);
threadCreator.start();
test(timeMax);
Copy link
Member

@dcubed-ojdk dcubed-ojdk Jun 3, 2022

Choose a reason for hiding this comment

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

Okay, but now the test is going to execute for twice timeMax which might be
a bit of a surprise... In particular the usage() mesg is now wrong.

Copy link
Member

@dcubed-ojdk dcubed-ojdk Jun 3, 2022

Choose a reason for hiding this comment

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

Also, the @bug line is not updated with this bug ID.

Copy link
Contributor Author

@pchilano pchilano Jun 3, 2022

Choose a reason for hiding this comment

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

Ok, we could fix this in a new bug.

}

public static void test(int timeMax) {
System.out.println("About to execute for " + timeMax + " seconds.");

long count = 0;