Fix:Idle event handler was called on every thread rather than thread ass... #337

Merged
merged 1 commit into from Jul 21, 2012

2 participants

@robwilkens

...igned on

Summary of changes in order git diff --staged is giving them to me:

(1) Each Xplat driver :
-local "Idle" variable remove, replaced with a Driver-level Idle_Threads
which is assigned by what is now the Idle Property.

-When Idle Called it used to do:
if (Idle != null)
Idle (this, e)

now we have to do some additional checks and change the call similar to as
follows to get the per-thread idle event handler:

(pseudo-pseudo-code)
id=ManagedThreadId;
if (Idle_Threads is not null and it contains a key for the id and the key
indexes into a non-null event handler) then
Idle_Threads [id] (this, e);

(2) Xplat Driver
Here's where the property is added and assigns the Idle event handler to
Idle_Threads. A Lock was needed the first time we create the Idle_Threads
dictionary. The Idle_Threads dictionary indexes by managed thread id and
maps onto an event handler for each one.

(3) Win32 Specific change
In my testing, I noticed that Win32 did not call Idle as part of its normal
Run loop processing. So, when it would call GetMessage and it was Blocking
we now check Idle if we were going to block. We don't actually call idle
if we first peekMessage and find a message, in those cases we just treat
it as if GetMessage Got the message. If we are blocking and peek returns
no messages, we call idle. If we have already called idle once on this
pass through the GetMessage Loop we set CheckIdle to false and go
back to the to to PRocessNextMEssage but this time we block rather than
peek and call idle.

(4) This includes a unit test.

This code was tested on Win32, MacOS/Carbon and Linux/X11. There have been
some minor formatting changes (Code style) since the testing, which should
not affect test results.

@robwilkens robwilkens Fix:Idle event handler was called on every thread rather than thread …
…assigned on

Summary of changes in order git diff --staged is giving them to me:

(1) Each Xplat driver :
-local "Idle" variable remove, replaced with a Driver-level Idle_Threads
which is assigned by what is now the Idle Property.

-When Idle Called from with thread it used to do:
if (Idle != null)
   Idle (this, e)

now we have to do some additional checks and change the call similar to as
follows to get the per-thread idle event handler:

(pseudo-pseudo-code)
id=ManagedThreadId;
if (Idle_Threads is not null and it contains a key for the id and the key
indexes into a non-null event handler) then
   Idle_Threads [id] (this, e);

(2) Xplat Driver
Here's where the property is added and assigns the Idle event handler to
Idle_Threads.  A Lock was needed the first time we create the Idle_Threads
dictionary.  The Idle_Threads dictionary indexes by managed thread id and
maps onto an event handler for each one.

(3) Win32 Specific change
In my testing, I noticed that Win32 did not call Idle as part of its normal
Run loop processing.  So, when it would call GetMessage and it was Blocking
we now check Idle if we were going to block.  We don't actually call idle
if we first peekMessage and find a message, in those cases we just treat
it as if GetMessage Got the message.  If we are blocking and peek returns
no messages, we call idle.  If we have already called idle once on this
pass through the GetMessage Loop we set CheckIdle to false and go
back to the to to PRocessNextMEssage but this time we block rather than
peek and call idle.

This code was tested on Win32, MacOS/Carbon and Linux/X11.  There have been
some minor formatting changes (Code style) since the testing, which should
not affect test results.
38bdbad
@migueldeicaza migueldeicaza merged commit dba2dd4 into mono:master Jul 21, 2012
@migueldeicaza
Mono Project member

I merged the patch so I could fix it, as currently it is incorrect, but it has been sitting here for so long, that it was unfair to request the patch to be redone with the changes.

There is a race condition in this patch: Idle is a static event, and thus it must be thread safe, which means that both the manipulation of the idle dictionary as well as the raising need to take locks.

@migueldeicaza
Mono Project member

Actually, I am going to revert this patch for two reasons:

(a) it would be better to have a single RaiseIdle function that is used by all backends to raise the Idle event, instead of having Win32 use that pattern, Cocoa and X11 manually peeking at the Idle_XX variables.

(b) It is not clear to me that we need to keep Idle handlers scoped to a particular thread, in fact, it looks wrong.

All idle handlers must be dispatched on the main thread. The current patch merely puts the idle handlers in different buckets, and merely avoids calling the idle handlers unless the mainloop is running on the same thread as the thread that was running the idle handler.

@robwilkens

Please note: The per-thread idle handlers are how it works in windows. If you create an idle handler in thead 1, it is only called in thread 1. If you create one in thread 3 it is only called when thread 3 goes idle.

I agree, though, that there may be race conditions.

Also, note that the Windows version of the idle handler does not call idle at all without this patch, from what i remember (it was a while ago i did this).

@slluis slluis pushed a commit to slluis/mono that referenced this pull request Feb 6, 2014
@migueldeicaza migueldeicaza Revert previous patch, see discussion on mono#337 3e8e365
@alexrp alexrp referenced this pull request Jun 20, 2014
@alexrp alexrp [MWF] Disable OneIdlePerThread test.
The code that made this test work was reverted, so this test is
bound to fail randomly.
6d69a06
@xen2 xen2 pushed a commit to xen2/mcs that referenced this pull request Aug 2, 2014
@migueldeicaza migueldeicaza Revert previous patch, see discussion on mono/mono#337 e1c3035
@richard-fine richard-fine referenced this pull request in Unity-Technologies/mono Dec 1, 2014
Merged

Managed thread id fix for 5.0 #180

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment