#4043 Tcl_Exit() shouldn't call the full Tcl_Finalize() path

obsolete: 8.6a0
closed-fixed
Kevin B KENNY
8
2009-07-22
2008-06-23
No

Tcl_Exit() often results in segfaults on exit, when more than 1 thread is busy at the time of a Tcl_Exit(). The problem is that Tcl_Finalize cleans up the state used by active threads. This problem has existed for a long time. The comments in Tcl_Finalize indicate the expectation of 1 thread "there really should only be one thread alive at this moment."

I propose this possible solution:
1. move the invoking in Tcl_Finalize of Tcl exit handlers into TclInvokeExitHandlers();

2. Tcl_Finalize can call TclInvokeExitHandlers()

3. Tcl_Exit calls TclInvokeExitHandlers() instead of Tcl_Finalize().

Tcl_Finalize will still work for those that wish to use it, and Tcl_Exit will work in the multiple active threads case.

Discussion

1 2 3 > >> (Page 1 of 3)
  • Logged In: YES
    user_id=585068
    Originator: YES

    I solved the problem in almost the way stated in this report. I had to call the channel finalization function, so that the Tcl_Channels get flushed in Tcl_Exit. It passes all tests now.
    File Added: tclEvent_ExitChange-4.patch

     
  • Logged In: YES
    user_id=585068
    Originator: YES

    I updated the patch for the HEAD, and now we have revision 5. The patch for some reason is more readable than the last, most likely due to whitespace changes.

    It's unclear to me whether or not we should have an inFinalize variable and use it within TclInitSubsystems for use with the previous Tcl_Panic pattern, that now only panics when inExit is true.

    File Added: tclEvent_ExitChange-5.patch

     
  • Logged In: YES
    user_id=585068
    Originator: YES

    Here is a simple Thread test case that faults before the patch is applied, due to the exit of the main thread causing the other threads to have invalid state during Tcl_Finalize, but before TclpExit.

    package require Thread

    set script {
    puts [time {
    set total 0 ; for {set i 0} {$i < 10000000} {incr i} {incr total $i} ; set total
    }]
    }

    set t1 [thread::create]
    set t2 [thread::create]

    thread::send -async $t1 $script
    thread::send -async $t2 $script

    ----
    $ tclsh8.6 tcl_thread_speed_test.tcl
    Segmentation fault (core dumped)

    (gdb) bt
    #0 0xb7e99a04 in __pthread_mutex_unlock_usercnt ()
    from /lib/tls/i686/cmov/libpthread.so.0
    #1 0xb7f9b74f in Tcl_MutexUnlock () from /usr/local/lib/libtcl8.6.so
    #2 0xb7f9c55f in Tcl_WaitForEvent () from /usr/local/lib/libtcl8.6.so
    #3 0xb7f64726 in Tcl_DoOneEvent () from /usr/local/lib/libtcl8.6.so
    #4 0xb7485027 in ThreadWaitObjCmd ()
    from /usr/local/lib/thread2.6.5/libthread2.6.5.so
    #5 0xfffffffd in ?? ()
    #6 0xb7fbf324 in ?? () from /usr/local/lib/libtcl8.6.so
    #7 0xb7fc65ec in tclTomMathConstStubsPtr () from /usr/local/lib/libtcl8.6.so
    #8 0xb7f6f4c0 in ?? () from /usr/local/lib/libtcl8.6.so
    Cannot access memory at address 0xfca3bad3

     
  • Logged In: YES
    user_id=585068
    Originator: YES

    Below is another test case that sometimes segfaults before the patch (the number of runs before a fault varies, but generally once every 5). After the patch I haven't been able to duplicate the problem.

    package require Thread

    thread::create {
    set i 0
    while 1 {
    puts $i
    incr i
    }
    }
    #now for the Tcl_Finalize race

     
    • assigned_to: andreas_kupries --> hobbs
     
  • Logged In: YES
    user_id=90580
    Originator: NO

    is the window-2.9 deadlock in threaded Tk relevant here? (Tk bugs 1715716 & 740570)

    I wrote there:

    > The problem is definitely caused by nested invocation of [exit] from the
    > <Destroy> handlers; not sure how to go about a fix in Tk, DestroyNotify
    > events explicitly ignore TK_ALREADY_DEAD...
    >
    > The easiest option might be to check inFinalize before setting it in
    > Tcl_Finalize(), or equivalently, to check TclInExit() in Tcl_Exit() before
    > calling Tcl_Finalize(), but I don't understand tcl finalization well enough
    > to know if that would have undesireable effects.

     
    • priority: 5 --> 7
     
  • Reassigning to kbk, because he has some time/interest in looking into this.

    I'm not ready yet to commit the change, because it seems too simple, and other developers haven't known the area as well.

     
    • assigned_to: hobbs --> kennykb
     
  • das, I'm not sure if the Tk deadlock is related. Can you test rev 5 of the patch with Tk to see if it fixes the deadlock with a threaded build?

     
1 2 3 > >> (Page 1 of 3)