Work at SourceForge, help us to make it a better place! We have an immediate need for a Support Technician in our San Francisco or Denver office.

Close

#5118 TclpFinalizeSockets hangs on windows

obsolete: 8.5.12
closed-fixed
Don Porter
9
2012-11-07
2012-10-04
Ralf Fassel
No

This seems related to the change introduced by ID: 1437595.

When running the attached code on Linux, the program opens the server
socket, waits one second and then exits as expected.

When running the same code on Windows-7 against tcl 8.3, the program
behaves as above on Linux.

When running the same code on Windows-7 against tcl 8.5 (Activestate,
or non-threaded makes no difference) the program blocks
in the _exit() call.

The reason is to be seen in TclFinalize() where on
Windows it waits for the socket thread to exit when unloading the TCL
DLL, but the corresponding thread either does no longer exist, or does
not get the Message, or does not respond to it:

win/tclWin32Dll.c:
DLLMain()
case DLL_PROCESS_DETACH:
...
Tcl_Finalize();
=>
generic/TclEvent.c
Tcl_Finalize()
=> Tcl_FinalizeThread()
=> TclFinalizeIOSubsystem();
=> TclpFinalizeSockets();

=>
win/tclWinSock.c
TclpFinalizeSockets()
=> PostMessage(tsdPtr->hwnd, SOCKET_TERMINATE, 0, 0);
// this blocks:
WaitForSingleObject(tsdPtr->readyEvent, INFINITE);

The PostMessage() return code is 0 at that point, indicating an error,
the corresponding error is 1400/invalid window handle.
If I change the code to

if (res) WaitForSingleObject(tsdPtr->readyEvent, INFINITE);

the program terminates as expected. It seems an error to call
WaitForSingleObject() if the PostMessage() call did not succeed, since
the Wait() relies on the other thread to signal the Wait().
If the other thread does not get the call, it cannot signal...

Proposed patch against tcl8.5.12:

--- tcl8.5.12/win/tclWinSock.c 2012/09/18 18:43:47 1.1
+++ tcl8.5.12/win/tclWinSock.c 2012/10/04 15:04:50
@@ -461,14 +461,14 @@
if (tsdPtr != NULL) {
if (tsdPtr->socketThread != NULL) {
if (tsdPtr->hwnd != NULL) {
- PostMessage(tsdPtr->hwnd, SOCKET_TERMINATE, 0, 0);
+ int res = PostMessage(tsdPtr->hwnd, SOCKET_TERMINATE, 0, 0);

/*
* Wait for the thread to exit. This ensures that we are
* completely cleaned up before we leave this function.
*/

- WaitForSingleObject(tsdPtr->readyEvent, INFINITE);
+ if (res) WaitForSingleObject(tsdPtr->readyEvent, INFINITE);
tsdPtr->hwnd = NULL;
}
CloseHandle(tsdPtr->socketThread);

Discussion

1 2 3 > >> (Page 1 of 3)
  • Ralf Fassel
    Ralf Fassel
    2012-10-04

    C source code

     
    Attachments
    t.c
    • assigned_to: nobody --> davygrvy
     
  • Sounds reasonable to me, but my knowledge of Windows (if any) is waning, so please Dave can you validate ?

     
  • Looks great!

     
    • labels: --> 27. Channel Types
    • priority: 5 --> 8
    • milestone: --> obsolete: 8.5.12
     
  • I'd be tempted to write that as:

    if (PostMessage(...)) {
    /* the comment... */
    WaitForSingleObject(...);
    }
    tsdPtr->hwnd = NULL;

    YMMV. At least the result of PostMessage() is defined to be boolean...

     
  • Ralf Fassel
    Ralf Fassel
    2012-10-05

    Of course the terse form Donal suggested is preferable. I had introduced the intermediate var before looking at the exact signature of PostMessage().
    And of course it would be nice to know *why* the handle is already invalid at that point, but...

     
  • Re "it would be nice to know *why*": agreed.
    Could you please set breakpoints/printfs at strategic places like SocketExitHandler , TclpFinalizeSocket, or after the "while" loop in SocketThread ? I suspect one of the first two is called twice.
    Also, what about 8.6 ?

     
  • unloading the DLL, for which a proper Tcl_Finalize() wasn't done before hand, is my guess. This usage of Tcl here is probably embedding and the app isn't being clean. I've seen this problem before with other areas of Tcl when the finalize logic is manipulated subtly, but has major affect in the abortive teardown.

     
  • Don Porter
    Don Porter
    2012-11-05

    Please report whether action on this is needed
    before an 8.5.13 release.

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