* unix/tclUnixSock.c (CreateClientSocket): Fix and simplify posting of the writable fileevent at the end of an asynchronous connection attempt. Improve comments for some of the trickery around [socket -async]. [Bug 3325339]

  *  tests/socket.test: Adjust tests to the async code changes. Add more tests for corner cases of async sockets.

3 files changed, 126 insertions, 26 deletions

diff --git a/ChangeLog b/ChangeLog
index f6909b1..de04d25 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,13 @@
+2011-06-28  Reinhard Max  <max@suse.de>
+
+	* unix/tclUnixSock.c (CreateClientSocket): Fix and simplify
+	posting of the writable fileevent at the end of an asynchronous
+	connection attempt. Improve comments for some of the trickery
+	around [socket -async]. [Bug 3325339]
+	
+	* tests/socket.test: Adjust tests to the async code changes. Add
+	more tests for corner cases of async sockets.
+
 2011-06-22  Andreas Kupries  <andreask@activestate.com>
 
 	* library/platform/pkgIndex.tcl: Updated to platform 1.0.10. Added
diff --git a/tests/socket.test b/tests/socket.test
index 7f5c5c2..363f141 100644
--- a/tests/socket.test
+++ b/tests/socket.test
@@ -1751,21 +1751,23 @@ test socket-14.1 {[socket -async] fileevent while still connecting} \
         }
         set server [socket -server accept -myaddr 127.0.0.1 0]
         set port [lindex [fconfigure $server -sockname] 2]
+        set x ""
     } -body {
         set client [socket -async localhost $port]
         fileevent $client writable {
-            lappend x [expr {[fconfigure $client -error] eq ""}]
+            lappend x [fconfigure $client -error]
         }
-        set after [after 1000 {set x timeout}]
-        vwait x
-        vwait x
-        set x
+        set after [after 1000 {lappend x timeout}]
+        while {[llength $x] < 2 && "timeout" ni $x} {
+            vwait x
+        }
+        lsort $x; # we only want to see both events, the order doesn't matter
     } -cleanup {
         after cancel $after
         close $server
         close $client
         unset x
-    } -result {ok 1}
+    } -result {{} ok}
 test socket-14.2 {[socket -async] fileevent connection refused} \
     -constraints [list socket supported_any] \
     -body {
@@ -1782,7 +1784,58 @@ test socket-14.2 {[socket -async] fileevent connection refused} \
         close $client
         unset x
     } -result "connection refused"
-
+test socket-14.3 {[socket -async] fileevent host unreachable} \
+    -constraints [list socket supported_any] \
+    -body {
+        # address from rfc5737
+        set client [socket -async 192.0.2.42 [randport]]
+        fileevent $client writable {set x [fconfigure $client -error]}
+        set after [after 5000 {set x timeout}]
+        vwait x
+        if {$x eq "timeout"} {
+            append x ": [fconfigure $client -error]"
+        }
+        set x
+    } -cleanup {
+        after cancel $after
+        close $client
+        unset x
+    } -result "host is unreachable"
+test socket-14.4 {[socket -async] and both, readdable and writable fileevents} \
+    -constraints [list socket supported_any] \
+    -setup {
+        proc accept {s a p} {
+            puts $s bye
+            close $s
+        }
+        set server [socket -server accept -myaddr 127.0.0.1 0]
+        set port [lindex [fconfigure $server -sockname] 2]
+        set x ""
+    } -body {
+        set client [socket -async localhost $port]
+        fileevent $client writable {
+            lappend x [fconfigure $client -error]
+            fileevent $client writable {}
+        }
+        fileevent $client readable {lappend x [gets $client]}
+        set after [after 1000 {lappend x timeout}]
+        while {[llength $x] < 2 && "timeout" ni $x} {
+            vwait x
+        }
+        lsort $x
+    } -cleanup {
+        after cancel $after
+        close $client
+        close $server
+    } -result {{} bye}
+test socket-14.5 {[socket -async] which fails before any connect() can be made} \
+    -constraints [list socket supported_any] \
+    -body {
+        # addresses from rfc5737
+        socket -async -myaddr 192.0.2.42 198.51.100.42 [randport]
+    } \
+    -returnCodes 1 \
+    -result {couldn't open socket: cannot assign requested address}
 ::tcltest::cleanupTests
 flush stdout
 return
diff --git a/unix/tclUnixSock.c b/unix/tclUnixSock.c
index 5ace251..52b089c 100644
--- a/unix/tclUnixSock.c
+++ b/unix/tclUnixSock.c
@@ -858,6 +858,17 @@ TcpGetHandleProc(
     return TCL_OK;
 }
 
+/*
+ *----------------------------------------------------------------------
+ *
+ * TcpAsyncCallback --
+ *
+ *	Called by the event handler that CreateClientSocket sets up
+ *	internally for [socket -async] to get notified when the
+ *	asyncronous connection attempt has succeeded or failed.
+ *
+ *----------------------------------------------------------------------
+ */
 static void
 TcpAsyncCallback(
     ClientData clientData,	/* The socket state. */
@@ -883,6 +894,18 @@ TcpAsyncCallback(
  * Side effects:
  *	Opens a socket.
  *
+ * Remarks:
+ *	A single host name may resolve to more than one IP address, e.g. for
+ *	an IPv4/IPv6 dual stack host. For handling asyncronously connecting
+ *	sockets in the background for such hosts, this function can act as a
+ *	coroutine. On the first call, it sets up the control variables for the
+ *	two nested loops over the local and remote addresses. Once the first
+ *	connection attempt is in progress, it sets up itself as a writable
+ *	event handler for that socket, and returns. When the callback occurs,
+ *	control is transferred to the "reenter" label, right after the initial
+ *	return and the loops resume as if they had never been interrupted.
+ *	For syncronously connecting sockets, the loops work the usual way.
+ *
  *----------------------------------------------------------------------
  */
 
@@ -892,14 +915,14 @@ CreateClientSocket(
     TcpState *state)
 {
     socklen_t optlen;
-    int in_coro = (state->addr != NULL);
+    int async_callback = (state->addr != NULL);
     int status;
     int async = state->flags & TCP_ASYNC_CONNECT;
 
-    if (in_coro) {
-        goto coro_continue;
+    if (async_callback) {
+        goto reenter;
     }
-    
+
     for (state->addr = state->addrlist; state->addr != NULL;
          state->addr = state->addr->ai_next) {
 
@@ -976,12 +999,13 @@ CreateClientSocket(
                                       TcpAsyncCallback, state);
                 return TCL_OK;
 
-            coro_continue:
+            reenter:
                 Tcl_DeleteFileHandler(state->fds.fd);
                 /*
-                 * Read the error state from the socket, to see if the async
-                 * connection has succeeded or failed and store the status in
-                 * the socket state for later retrieval by [fconfigure -error]
+                 * Read the error state from the socket to see if the async
+                 * connection has succeeded or failed. As this clears the
+                 * error condition, we cache the status in the socket state
+                 * struct for later retrieval by [fconfigure -error].
                  */
                 optlen = sizeof(int);
                 getsockopt(state->fds.fd, SOL_SOCKET, SO_ERROR,
@@ -996,22 +1020,35 @@ CreateClientSocket(
 
 out:
 
-    if (async) {
+    if (async_callback) {
+        /*
+         * An asynchonous connection has finally succeeded or failed.
+         */
         CLEAR_BITS(state->flags, TCP_ASYNC_CONNECT);
         TcpWatchProc(state, state->filehandlers);
         TclUnixSetBlockingMode(state->fds.fd, TCL_MODE_BLOCKING);
-    }
 
-    if (status < 0) {
-        if (in_coro) {
-            Tcl_NotifyChannel(state->channel, TCL_WRITABLE);
-        } else {
-            if (interp != NULL) {
-                Tcl_AppendResult(interp, "couldn't open socket: ",
-                                 Tcl_PosixError(interp), NULL);
-            }
-            return TCL_ERROR;
+        /*
+         * We need to forward the writable event that brought us here, bcasue
+         * upon reading of getsockopt(SO_ERROR), at least some OSes clear the
+         * writable state from the socket, and so a subsequent select() on
+         * behalf of a script level [fileevent] would not fire. It doesn't
+         * hurt that this is also called in the successful case and will save
+         * the event mechanism one roundtrip through select().
+         */
+        Tcl_NotifyChannel(state->channel, TCL_WRITABLE);
+
+    } else if (status != 0) {
+        /*
+         * Failure for either a synchronous connection, or an async one that
+         * failed before it could enter background mode, e.g. because an
+         * invalid -myaddr was given.
+         */
+        if (interp != NULL) {
+            Tcl_AppendResult(interp, "couldn't open socket: ",
+                             Tcl_PosixError(interp), NULL);
         }
+        return TCL_ERROR;
     }
     return TCL_OK;
 }