Tcl package Thread source code

View Ticket
Login
Bounty program for improvements to Tcl and certain Tcl packages.
Tcl 2019 Conference, Houston/TX, US, Nov 4-8
Send your abstracts to [email protected]
or submit via the online form by Sep 9.
Ticket UUID: b5709ea9060d17f58ba48110351c964b3408e362
Title: [::thread::send -async] posting order not respected when sending to current thread
Type: Bug Version: 2.8.0
Submitter: adrianmedranocalvo Created on: 2018-07-31 11:27:59
Subsystem: 80. Thread Package Assigned To: nobody
Priority: 5 Medium Severity: Important
Status: Open Last Modified: 2018-08-10 10:37:55
Resolution: None Closed By: nobody
    Closed on:
Description:
if {0} {
When the target thread of [::thread::send -async] is the current thread, the
async script is not evaluated in order with respect to other [::thread::send
-async] scripts.

Let's call a [::thread::send -async] invoked from the current thread and
directed to the current thread a local [::thread::send -async], and a
[::thread::send -async] invoked from any other thread and directed to the former
thread a remote [::thread::send -async].

Current behaviour is that all remote [::thread::send -async] scripts will be
evaluated before any local [::thread::send -asinc] scripts are, even in the case
where the local scripts have been posted before the remote ones.  See the script
below for a concise example.

Intuitively, [::thread::send -async] scripts should be queued and evaluated in
the order they were added to the queue (that is, except if the -head argument
was given).

The script below demostrates this issue.  We append to a global variable in
the main thread in four different ways, in order:
- An after 0 script
- An after idle script
- A local [thread::send -async]
- A remote [thread::send -async]
- Another after 0 script
- Another after idle script

The printed trace of the script is reproduced below.  Contrary to the posting
order, the remote [thread::send -async] is evaluated before the local
[thread::send -async].

~~~
REMOTE-THREAD-SEND
AFTER0-BEG
AFTER0-END
IDLE-BEG
LOCAL-THREAD-SEND
IDLE-END
~~~

Without knowing the details of the Thread package, one would expect the local
and remote [thread::send -async] to be evaluated in the order they were
posted.  Instead, the remote [thread::send -async] are invariably evaluated
before the local ones; and the local ones are evaluated as [after idle] commands.

The cause for this behaviour is an optimization in threadCmd.c, commented as
"Short circuit sends to ourself", where local [thread::send -async] are
converted to [after idle], implying that a different queue is used for local
[thread::send -async] events as oposed to remote [thread::send -async] events.

How important is this optimization?  I can't see much performance being lost if
we would disable the optimization and would let local [::thread::send -async] be
added to the queue along with all others.  The benefit from this change would be
a more consistent behaviour.
    
The underlying use-case is a server with connection-specific threads.  The
connection-specific thread uses local [thread::send -async] in order to delay
the flushing of responses, so that the application's locks are grabbed for a
minimum amount of time.  Other threads use remote [thread::send -async] to the
connection thread in order to send messages through the connection.  The
optimization above difficults ensuring the order the responses are flushed.

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
}

package require Thread;

set t(main) [::thread::id]
set t(other) [::thread::create]

# Let t(other) know the main thread's id, t(main).
::thread::send $t(other) [list array set t [array get t]]

after 0 {lappend ::trace AFTER0-BEG;}
after idle {lappend ::trace IDLE-BEG;}

::thread::send -async $t(main) {
    lappend ::trace LOCAL-THREAD-SEND;
}
::thread::send $t(other) {
    ::thread::send -async $t(main) {
	lappend ::trace REMOTE-THREAD-SEND;
    }
}

after 0 {lappend ::trace AFTER0-END;}
after idle {lappend ::trace IDLE-END;}

after 100 {set forever now};
vwait forever;
puts [join $::trace "\n"];

# ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
User Comments: adrianmedranocalvo (claiming to be [email protected]) added on 2018-08-10 10:37:55:
Hello,

Implemented the suggested change, please see patch below.  Tests pass:

~~~
Only running tests that match:  *
Skipping test files that match:  l.*.test
Only sourcing test files that match:  *.test
Tests began at Fri Aug 10 11:57:28 CEST 2018
Thread 2.8.0
Mainthread id is tid0x7fff8e159380
thread.test
tkt-84be1b5a73.test

tpool.test
tsv.test
ttrace.test

Tests ended at Fri Aug 10 11:57:43 CEST 2018
all.tcl:        Total   136     Passed  120     Skipped 16      Failed  0
Sourced 0 Test Files.
Number of tests skipped for each constraint:
        8       have_gdbm
        8       have_lmdb
~~~

~~~
diff --git c/src/tcl8.6.6/pkgs/thread2.8.0/generic/threadCmd.c i/src/tcl8.6.6/pkgs/thread2.8.0/generic/threadCmd.c
index 9744230c3..b9af96475 100644
--- c/src/tcl8.6.6/pkgs/thread2.8.0/generic/threadCmd.c
+++ i/src/tcl8.6.6/pkgs/thread2.8.0/generic/threadCmd.c
@@ -343,9 +343,6 @@ ThreadErrorProc(Tcl_Interp *interp);
 static void
 ThreadFreeProc(ClientData clientData);
 
-static void
-ThreadIdleProc(ClientData clientData);
-
 static void
 ThreadExitProc(ClientData clientData);
 
@@ -2697,21 +2694,13 @@ ThreadSend(interp, thrId, send, clbk, flags)
     }
 
     /*
-     * Short circuit sends to ourself.
+     * Short circuit synchronous sends to ourself.
      */
-
-    if (thrId == Tcl_GetCurrentThread()) {
+    if (thrId == Tcl_GetCurrentThread() && (flags & THREAD_SEND_WAIT)) {
         Tcl_MutexUnlock(&threadMutex);
-        if ((flags & THREAD_SEND_WAIT)) {
-            int code = (*send->execProc)(interp, (ClientData)send);
-            ThreadFreeProc((ClientData)send);
-            return code;
-        } else {
-            send->interp = interp;
-            Tcl_Preserve((ClientData)send->interp);
-            Tcl_DoWhenIdle((Tcl_IdleProc*)ThreadIdleProc, (ClientData)send);
-            return TCL_OK;
-        }
+        int code = (*send->execProc)(interp, (ClientData)send);
+        ThreadFreeProc((ClientData)send);
+        return code;
     }
 
     /*
@@ -3462,34 +3451,6 @@ ThreadSetOption(interp, thrId, option, value)
     return TCL_OK;
 }
 
-/*
- *----------------------------------------------------------------------
- *
- * ThreadIdleProc --
- *
- * Results:
- *
- * Side effects.
- *
- *----------------------------------------------------------------------
- */
-
-static void
-ThreadIdleProc(clientData)
-    ClientData clientData;
-{
-    int ret;
-    ThreadSendData *sendPtr = (ThreadSendData*)clientData;
-
-    ret = (*sendPtr->execProc)(sendPtr->interp, (ClientData)sendPtr);
-    if (ret != TCL_OK) {
-        ThreadErrorProc(sendPtr->interp);
-    }
-
-    Tcl_Release((ClientData)sendPtr->interp);
-    ThreadFreeProc(clientData);
-}
-
 /*
  *----------------------------------------------------------------------
  *
~~~

adrianmedranocalvo (claiming to be [email protected]) added on 2018-08-01 07:53:26:
Hello sebres,

great!

Indeed, the code you selected is correct, and mine was wrong.  Sorry for the confusion.

Let me know if you need any help.

Best regards,
Adrián.

sebres added on 2018-07-31 16:55:21:

Now I understand your objection (I was just confused with both after(timer) events from your example and your description).

Indeed it is possible..., but not what you provided as block "to remove", this one would be enough to remove:

61003585d6458178(lines 2752-2756)

Of course with move of `Tcl_MutexUnlock(&threadMutex);` (line 2747) 2 lines down, into block `if ((flags & THREAD_SEND_WAIT))`.

I'll take a look later as regards the test-cases and some other dependencies and behavior etc.


adrianmedranocalvo (claiming to be [email protected]) added on 2018-07-31 16:22:35:
Hello sebres,

there's definitely some misunderstanding.  English is not my mother language, it's not the first time I cause a mess...

I'll try to explain with code.

One important point is: I don't care about the order of [after idle] events nor [after 0] events with respect to [thread::send -async] events.  I only care about the order of [thread::send -async] events.

Please, try the following script:

~~~
package require Thread;

set t(main) [::thread::id]
set t(other) [::thread::create]

# Let t(other) know the main thread's id, t(main).
::thread::send $t(other) [list array set t [array get t]]

# Async thread::send to current thread (the main thread).
# I expect this script to be inserted at the end of the thread event queue.
::thread::send -async $t(main) {
    lappend ::trace LOCAL-THREAD-SEND;
}

::thread::send $t(other) {
    # Async thread::send to other thread (the main thread).
    # I expect this script to be inserted at the end of the
    # thread event queue, after the one above.
    ::thread::send -async $t(main) {
	  lappend ::trace REMOTE-THREAD-SEND;
    }
}

after 100 {set forever 1};
vwait forever;
puts [join $::trace "\n"];
~~~

This is the output:

~~~
REMOTE-THREAD-SEND
LOCAL-THREAD-SEND
~~~

This is not expected, because the [thread::send -async] lappending LOCAL-THREAD-SEND ran before the one lappending REMOTE-THREAD-SEND.

When I remove the following code:
  https://core.tcl-lang.org/thread/artifact?udc=1&ln=2742-2759&name=61003585d6458178&udc=1

I obtain the expected output:

~~~
LOCAL-THREAD-SEND
REMOTE-THREAD-SEND
~~~

Are there any downsides in removing the following code?
  https://core.tcl-lang.org/thread/artifact?udc=1&ln=2742-2759&name=61003585d6458178&udc=1

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

I'll now try to respond to your points.

> if you'll do that, the event will be appended directly into the queue of the thread, so in this case it will be not LAST (as by idle) but FIRST event (because timer events coming later via processing of event-sources).
>
> So again, it cannot be "on the order they were posted", because they are another types of events. It will be not between your timer-events, but either hereafter (like now) or before (if direct appended to event-queue). But definitely not between.

I think that's what I want and expect, that the [thread::send -async] is appended to the queue of the thread.  I don't expect them to be handled between other types of events.

> For this case "thread::send -async [thread::id]" should be quasi rewritten as "after 0", and I'm definitely against this, because it produces unexpected overhead.

I'm also against this.  In fact that's the precise problem: "thread::send -async [thread::id]" is being rewritten into "after idle".  See the part of the code where that happens (https://core.tcl-lang.org/thread/artifact?udc=1&ln=2742-2759&name=61003585d6458178&udc=1):

  2742      /*
  2743       * Short circuit sends to ourself.
  2744       */
  2745  
  2746      if (thrId == Tcl_GetCurrentThread()) {
  2747          Tcl_MutexUnlock(&threadMutex);
  2748          if ((flags & THREAD_SEND_WAIT)) {
  2749              int code = (*send->execProc)(interp, (ClientData)send);
  2750              ThreadFreeProc((ClientData)send);
  2751              return code;
  2752          } else {
  2753              send->interp = interp;
  2754              Tcl_Preserve((ClientData)send->interp);
  2755              Tcl_DoWhenIdle((Tcl_IdleProc*)ThreadIdleProc, (ClientData)send);
  2756              return TCL_OK;
  2757          }
  2758      }

> Possibly for my another event-branch (that I wrote in tcl-core-repo), where "after 0" have O(1) by changing of timer-queue, does not generate extra overhead as regards time-compare at all (because creates an immediate timer-event), etc.
If this will be accepted sometimes... 

More performant event handling is always welcome... thank you for your efforts, I hope it gets merged soon.

sebres added on 2018-07-31 15:44:54:

But you did still not understand...

if you'll do that, the event will be appended directly into the queue of the thread, so in this case it will be not LAST (as by idle) but FIRST event (because timer events coming later via processing of event-sources).

So again, it cannot be "on the order they were posted", because they are another types of events. It will be not between your timer-events, but either hereafter (like now) or before (if direct appended to event-queue). But definitely not between.

For this case "thread::send -async [thread::id]" should be quasi rewritten as "after 0", and I'm definitely against this, because it produces unexpected overhead.

Possibly for my another event-branch (that I wrote in tcl-core-repo), where "after 0" have O(1) by changing of timer-queue, does not generate extra overhead as regards time-compare at all (because creates an immediate timer-event), etc.
If this will be accepted sometimes...


adrianmedranocalvo (claiming to be [email protected]) added on 2018-07-31 14:35:32:
Hello sebres,

thank you for your comments.

Indeed that's the issue: some [thread::send -async] are being transformed into "after" events, which implies all points you detailed, while other [thread::send -async] do not.

Mine was a lengthy text, I'm not sure my proposal it's clear.  Let me summarize it:

- I think that each "thread::send -async $tid $script" should be appended to the same queue, so that events for thread $tid are evaluated on the order they were posted.

- Other events can be handled in whatever order.

- I think that transforming some "thread::send -async $tid $script" into "after idle" (as it's currently done in Thread 2.8.0) is a misfeature, because it makes "thread::send -async" no longer work as a useful thread-safe queue.

- I propose removing the code selected in the following link:
  https://core.tcl-lang.org/thread/artifact?udc=1&ln=2742-2759&name=61003585d6458178&udc=1

What's your opinion on doing that?

sebres added on 2018-07-31 12:22:41:

Although idle-event is indeed not really good strategy for async send to self.

But I don't see this as issue.

Please note:

  • "after" events are timer-events and will be executed as one chunk (if the timer was reached, and in case of after 0 it is always the case);
  • this means - you can not insert in-between 0-timer events, unless you don't generate a timer-event also (I don't really see any advantages by generation of 0-timer event in case of async send).
  • events are cumulated with event source handlers (setupProc/checkProc), so the time where events are reached the evaluation phase is undefined;
  • if the event will be appended directly to thread-queue, then all your timer-events (after 0) will be executed hereafter (because those will reach the event-queue first after sourcing via setupProc/checkProc);
  • asynchronous send to self is not the primary usage in thread-module, so I don't think that "much performance being lost" here.

Just as loud thoughts.