harmony-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Nikolay Kuznetsov (JIRA)" <j...@apache.org>
Subject [jira] Commented: (HARMONY-2217) [drlvm] Improve interrupt() implementation
Date Mon, 27 Nov 2006 15:43:22 GMT
    [ http://issues.apache.org/jira/browse/HARMONY-2217?page=comments#action_12453623 ] 
Nikolay Kuznetsov commented on HARMONY-2217:

first of all thank you so much for your efforts in resolving HARMONY-1789 issue. I'm sorry
for missing PARKED stated and including multiple issues(appeared during work on 1789) into
the single patch.

But let me comment here on interrupter thread purpose.

Initially 1789 was intended to synchronize setting interrupted state and falling to multiple
kind of wait, namely sleep(), park() and Object.wait() (initially interruption may have happened
just after interrupted state check, but before falling into actual wait and never be delivered
to the waiting thread). As you have noticed all the wait functions based on condition variable
and according to wait specification interrupted state should be checked twice: just before
wait and after(for the case thread was interrupted during wait). The most natural way to implement
this is to acquire thread local mutex on wait operation, release on condition wait(pass this
mutex to the cond_wait) and on wait exit, thus interrupted state would have been always synchronized.
This works fine for sleep and park methods, but not for Object.wait, because Object.wait waits
on it's own mutex and you can't atomically release both mutexes (thread local and monitors)
and this may allow interruption to happen between thread->mutex release and monitor_wait.

In my version of the patch to 1789 in case of Object.wait interrupted state was guarded by
monitor itself, and notification would have been delivered even if interruped state was set
between interrupted status check in condition and falling into wait. The interrupter thread
in its turn was required to eliminate blocking on interruption, because target monitor may
have been already acquired by a third thread.

After you patch interruption may have happened right after thread->mutex releasing + interrupted
state check, but before falling into wait, thus interrupt will not wake waiting monitor.

thread_native_fat_monitor.c (monitor_wait_impl):
hymutex_unlock(self->mutex);<!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! mutex released

status =  condvar_wait_impl(mon_ptr->condition, mon_ptr->mutex, ms, nano, interruptable);

thread_native_condvar.c (wait_impl):
if (interruptable && (this_thread->state & TM_THREAD_STATE_INTERRUPTED)) {
        // clean interrupted flag
        this_thread->state &= (~TM_THREAD_STATE_INTERRUPTED);
                return TM_ERROR_INTERRUPT;

Interruption unsafe place, not covered by any lock.
apr_thread_cond_wait((apr_thread_cond_t*)cond, (apr_thread_mutex_t*)mutex);

Thus I believe that removing synchronization for Object.wait is incorrect and should be rolled
back or different approach should be discussed.
I understand that this explanation may seem quite vague and may require additional explanation,
I'm open answer any questions.

Thank you.

> [drlvm] Improve interrupt() implementation
> ------------------------------------------
>                 Key: HARMONY-2217
>                 URL: http://issues.apache.org/jira/browse/HARMONY-2217
>             Project: Harmony
>          Issue Type: Sub-task
>          Components: DRLVM
>            Reporter: Salikh Zakirov
>         Assigned To: Alexey Varlamov
>            Priority: Minor
>         Attachments: 0001-removed-interrupter-thread.patch, 0002-refactored-wait_count-to-be-modified-only-from-wait.patch,
> This issue contains improvements of interrupt() implementation on top of H-1789-updated.patch
from HARMONY-1789:
> 0001-removed-interrupter-thread.patch replaces the auxiiliary interrupter thread with
direct call to hycond_notify_all(), since this call does not require the monitor to be held
> 0002-refactored-wait_count-to-be-modified-only-from-wait.patch fixes wait_count handling,
which is broken a bit by 0001 patch. IMHO, modifying wait_count outside of threads waiting
on the monitor was not correct in the first place.
> 0003-replaced-HyThread.monitor-with-HyThread.current_condition.patch further simplifies
interrupt() implementation by removing special case for a thread waiting on a monitor, since
it boils down to waiting on a conditional variable.

This message is automatically generated by JIRA.
If you think it was sent incorrectly contact one of the administrators: http://issues.apache.org/jira/secure/Administrators.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira


View raw message