On Fri, 19 Nov 2021 17:04:42 GMT, Daniel D. Daugherty <dcu...@openjdk.org>
wrote:
>> Okay. I see similar check in the `force_early_return()` function:
>>
>> if (state == NULL) {
>> return JVMTI_ERROR_THREAD_NOT_ALIVE;
>> }
>>
>> Would it better to replace it with this check instead? :
>>
>> if (java_thread->is_exiting()) {
>> return JVMTI_ERROR_THREAD_NOT_ALIVE;
>> }
>>
>> Removing this check and keep the one inside the handshake would be even
>> better.
>>
>> I would also add this line for symmetry with two other cases:
>>
>> + MutexLocker mu(JvmtiThreadState_lock);
>> SetForceEarlyReturn op(state, value, tos);
>
> My point is that I don't see why you added the `is_exiting()` check
> since I don't see a race in that function, i.e., there's no `assert()` in
> this function that you need to protect.
>
> As for adding the `MutexLocker mu(JvmtiThreadState_lock)`, you'll
> have to analyze and justify why you would need to add that lock grab
> independent of this fix. I'm not seeing a bug there, but I haven't looked
> very closely.
The `is_exiting` check changes the behaviour from reporting
JVMTI_ERROR_THREAD_NOT_SUSPENDED to JVMTI_ERROR_THREAD_NOT_ALIVE. Arguably it
is a more precise answer, but it is somewhat splitting hairs. To me it might be
clearer to the developer what their logic error is if they get NOT_SUSPENDED
rather than NOT_ALIVE. Either way this change is not needed to fix any known
bug and the change is behaviour seems questionable.
>> Dan,
>> Thank you for reviewing this!
>> I'm not sure, I correctly understand you here.
>> Are you saying that you agree with this change?
>> In fact, the thread state can not be changed (and the assert fired) after
>> the `is_exiting()` check is made even without `JvmtiThreadState_lock`
>> protection because it is inside of a handshake execution.
>
> I agree with the `is_exiting()` check addition.
>
> I forgot that we're executing a Handshake `doit()` function. So we have a
> couple
> of reasons why an unsuspended target thread can't change from `!is_exiting()`
> to `is_exiting()` while we are in this function.
Again this introduces a more precise state check but also changes the behaviour
by now reporting NOT_ALIVE instead of NOT_SUSPENDED. The assertion failure can
be fixed by simply moving the assertion to after the suspension check.
-------------
PR: https://git.openjdk.java.net/jdk/pull/6440
