Wapp

View Ticket
Login
Ticket UUID: 207094dd9de1343af9d874b8562fc1b4169c3e44
Title: Multiple request in progress can step on each others toes
Status: Fixed Type: Code_Defect
Severity: Important Priority: Immediate
Subsystem: Resolution: Fixed
Last Modified: 2019-07-31 05:08:38
Version Found In:
User Comments:
mjanssen added on 2019-07-30 10:26:14:

Steps to reproduce:

  • Define a request handler which takes a noticable time to be handled.
  • Fire the request to this handler as fast as possible.

Wapp will eventually respond with a large number of errors. Either:

can't unset "::wappInt-sock7fcc0e829d90": no such variable
    while executing
"unset ::wappInt-$chan"

Or

can not find channel named "sock7fcc0e829d90"
    while executing
"puts $chan "HTTP/1.1 [dict get $wapp .reply-code]\r"

This seems to be cause by the use of the global ::wappInt-$chan variables. Tracking the request parsing state in the fileevent fixes this issue.


mjanssen added on 2019-07-30 10:29:17:
diff --git a/modules/wapp-1.0.tm b/modules/wapp-1.0.tm
index 0467333..57888d1 100644
--- a/modules/wapp-1.0.tm
+++ b/modules/wapp-1.0.tm
@@ -426,7 +426,6 @@ proc wappInt-start-browser {url} {
 # request if $fromip is not an empty string and does not match $ip.
 #
 proc wappInt-new-connection {callback wappmode fromip chan ip port} {
-  upvar #0 wappInt-$chan W
   if {$fromip!="" && ![string match $fromip $ip]} {
     close $chan
     return
@@ -434,7 +433,7 @@ proc wappInt-new-connection {callback wappmode fromip chan ip port} {
   set W [dict create REMOTE_ADDR $ip REMOTE_PORT $port WAPP_MODE $wappmode \
          .header {}]
   fconfigure $chan -blocking 0 -translation binary
-  fileevent $chan readable [list $callback $chan]
+  fileevent $chan readable [list $callback $W $chan]
 }
 
 # Close an input channel
@@ -444,21 +443,20 @@ proc wappInt-close-channel {chan} {
     # This happens after completing a CGI request
     exit 0
   } else {
-    unset ::wappInt-$chan
     close $chan
   }
 }
 
 # Process new text received on an inbound HTTP request
 #
-proc wappInt-http-readable {chan} {
-  if {[catch [list wappInt-http-readable-unsafe $chan] msg]} {
+proc wappInt-http-readable {W chan} {
+  if {[catch [list wappInt-http-readable-unsafe $W $chan] msg]} {
     puts stderr "$msg\n$::errorInfo"
     wappInt-close-channel $chan
   }
 }
-proc wappInt-http-readable-unsafe {chan} {
-  upvar #0 wappInt-$chan W wapp wapp
+proc wappInt-http-readable-unsafe {W chan} {
+  upvar #0 wapp wapp
   if {![dict exists $W .toread]} {
     # If the .toread key is not set, that means we are still reading
     # the header
@@ -481,10 +479,7 @@ proc wappInt-http-readable-unsafe {chan} {
         dict set W SCRIPT_FILENAME $a0
         dict set W DOCUMENT_ROOT [file dir $a0]
       }
-      if {[wappInt-parse-header $chan]} {
-        catch {close $chan}
-        return
-      }
+      set W [wappInt-parse-header $W] 
       set len 0
       if {[dict exists $W CONTENT_LENGTH]} {
         set len [dict get $W CONTENT_LENGTH]
@@ -496,6 +491,7 @@ proc wappInt-http-readable-unsafe {chan} {
         # There is no query content, so handle the request immediately
         set wapp $W
         wappInt-handle-request $chan 0
+        return
       }
     }
   } else {
@@ -508,8 +504,10 @@ proc wappInt-http-readable-unsafe {chan} {
       # Handle the request as soon as all the query content is received
       set wapp $W
       wappInt-handle-request $chan 0
-    }
+      return
+    } 
   }
+  fileevent $chan readable [list wappInt-http-readable $W $chan]
 }
 
 # Decode the HTTP request header.
@@ -517,8 +515,7 @@ proc wappInt-http-readable-unsafe {chan} {
 # This routine is always running inside of a [catch], so if
 # any problems arise, simply raise an error.
 #
-proc wappInt-parse-header {chan} {
-  upvar #0 wappInt-$chan W
+proc wappInt-parse-header {W} {
   set hdr [split [dict get $W .header] \n]
   if {$hdr==""} {return 1}
   set req [lindex $hdr 0]
@@ -555,7 +552,7 @@ proc wappInt-parse-header {chan} {
     }
     dict set W $name $value
   }
-  return 0
+  return $W
 }
 
 # Decode the QUERY_STRING parameters from a GET request or the
@@ -618,6 +615,10 @@ proc wappInt-decode-query-params {} {
 #
 proc wappInt-handle-request {chan useCgi} {
   global wapp
+  if {$chan ni [chan names]} {
+    return
+  }
+  fileevent $chan readable {}
   dict set wapp .reply {}
   dict set wapp .mimetype {text/html; charset=utf-8}
   dict set wapp .reply-code {200 Ok}
@@ -818,14 +819,13 @@ proc wappInt-handle-cgi-request {} {
 
 # Process new text received on an inbound SCGI request
 #
-proc wappInt-scgi-readable {chan} {
-  if {[catch [list wappInt-scgi-readable-unsafe $chan] msg]} {
+proc wappInt-scgi-readable {W chan} {
+  if {[catch [list wappInt-scgi-readable-unsafe $W $chan] msg]} {
     puts stderr "$msg\n$::errorInfo"
     wappInt-close-channel $chan
   }
 }
-proc wappInt-scgi-readable-unsafe {chan} {
-  upvar #0 wappInt-$chan W wapp wapp
+proc wappInt-scgi-readable-unsafe {W chan} {
   if {![dict exists $W .toread]} {
     # If the .toread key is not set, that means we are still reading
     # the header.
@@ -855,6 +855,7 @@ proc wappInt-scgi-readable-unsafe {chan} {
       dict set W SERVER_ADDR [dict get $W .remove_addr]
       set wapp $W
       wappInt-handle-request $chan 0
+      return
     }
   } else {
     # If .toread is set, that means we are reading the query content.
@@ -867,8 +868,10 @@ proc wappInt-scgi-readable-unsafe {chan} {
       dict set W SERVER_ADDR [dict get $W .remove_addr]
       set wapp $W
       wappInt-handle-request $chan 0
+      return
     }
   }
+  fileevent $chan readable [list wappInt-scgi-readable $W $chan]
 }
 
 # Start up the wapp framework.  Parameters are a list passed as the

drh added on 2019-07-30 16:30:34:

I am unable to reproduce the problem on ubuntu. I created a small Wapp application (derived from examples/fileupload.tcl) that adds a page named "slow" that does a loop of 10 million iterations. I ran that script with "tclsh fileup1.tcl --server 8080" and then hit it with 20 continuous parallel HTTP requests using "http_load". Running this for 1000 requests produced no errors.

I tried adding the patch, but the patch fails for CGI. I did not attempt to debug the patch.


mjanssen added on 2019-07-30 17:02:27:
I am testing this from the browser. The fact that the client end is gone might be related. I will look at the patch for CGI.

mjanssen added on 2019-07-30 17:03:59:

To clarify, I keep refreshing a slow page really quickly. So the client socket is closed when wapp gets to it.


drh added on 2019-07-30 17:56:45:

Here is a test Wapp script I am using:

source ./wapp.tcl
proc wapp-default {} {
  wapp-trim {
    <p>Visit the <a href='%html([wapp-param BASE_URL]/slow)'>slow</a>
    page for a slow-to-load case.</p>
  }
}
proc wapp-page-slow {} {
  set x [expr {abs(int(rand()*100000))}]
  for {set i 0} {$i<20000000} {incr i} {
    set x [expr {$x+1}]
  }
  wapp-trim {
    <h1>Slow page</h1>
    <p>x = %html($x)</p>
  }
}
wapp-start $argv

I run this as "tclsh slow.tcl --server 8080" and then look at the /slow page in a webbrowser. Then I start hammering the refresh button. Everything works fine for me. No errors. (Tcl 8.7, Firefox, Ubuntu)

Do you have another test case that will better demonstrate the problem?


mjanssen added on 2019-07-30 19:30:38:
It's some weird interaction with the Tcl http package.

Your adapted script below reproduces the issue for me whereas your original script doesn't.

source ./wapp.tcl
###
package require http
proc wapp-default {} {
  wapp-trim {
    <p>Visit the <a href='%html([wapp-param BASE_URL]/slow)'>slow</a>
    page for a slow-to-load case.</p>
  }
}
proc wapp-page-slow {} {
  ###
  set tok [http::geturl "http://www.google.com"]
  http::cleanup $tok
  set x [expr {abs(int(rand()*100000))}]
  for {set i 0} {$i<20000000} {incr i} {
    set x [expr {$x+1}]
  }
  wapp-trim {
    <h1>Slow page</h1>
    <p>x = %html($x)</p>
  }
}
wapp-start $argv

drh added on 2019-07-30 23:14:28:

The problem was that http was invoking the event-loop which was causing the wappInt-handle-request proc to be invoked recursively. But that routine uses a global variable (::wapp) and cannot be invoked recursively.

The fix was to serialize access to wappInt-handle-request by queuing recursive calls and executing them in order after the previous finishes.


mjanssen added on 2019-07-31 05:08:38:

Good find and thanks for the fix. I still don't quite understand why taking the state for a single connection in the readable callback instead of in the wappInt-.. global also seemed to fix the issue, but that is probably more luck than anything else.