Unroll `defer`s and remove `break`s. (#440)

* Unpack `finally/defer` blocks and introduce explicit cleaning of objects.
Add request query to debug information.

* Unroll one more loop to avoid `break`.
Add test for query debug string.

* Fix cancellation behavior.

* Address review comments.

chronos/apps/http/httpdebug.nim

@@ -29,6 +29,7 @@ type
     handle*: SocketHandle
     connectionType*: ConnectionType
     connectionState*: ConnectionState
+    query*: Opt[string]
     remoteAddress*: Opt[TransportAddress]
     localAddress*: Opt[TransportAddress]
     acceptMoment*: Moment
@@ -85,6 +86,12 @@ proc getConnectionState*(holder: HttpConnectionHolderRef): ConnectionState =
   else:
     ConnectionState.Accepted
 
+proc getQueryString*(holder: HttpConnectionHolderRef): Opt[string] =
+  if not(isNil(holder.connection)):
+    holder.connection.currentRawQuery
+  else:
+    Opt.none(string)
+
 proc init*(t: typedesc[ServerConnectionInfo],
            holder: HttpConnectionHolderRef): ServerConnectionInfo =
   let
@@ -98,6 +105,7 @@ proc init*(t: typedesc[ServerConnectionInfo],
         Opt.some(holder.transp.remoteAddress())
       except CatchableError:
         Opt.none(TransportAddress)
+    queryString = holder.getQueryString()
 
   ServerConnectionInfo(
     handle: SocketHandle(holder.transp.fd),
@@ -106,6 +114,7 @@ proc init*(t: typedesc[ServerConnectionInfo],
     remoteAddress: remoteAddress,
     localAddress: localAddress,
     acceptMoment: holder.acceptMoment,
+    query: queryString,
     createMoment:
       if not(isNil(holder.connection)):
         Opt.some(holder.connection.createMoment)

chronos/apps/http/httpserver.nim

@@ -148,6 +148,7 @@ type
     writer*: AsyncStreamWriter
     closeCb*: HttpCloseConnectionCallback
     createMoment*: Moment
+    currentRawQuery*: Opt[string]
     buffer: seq[byte]
 
   HttpConnectionRef* = ref HttpConnection
@@ -813,6 +814,7 @@ proc closeUnsecureConnection(conn: HttpConnectionRef) {.async.} =
     except CancelledError:
       await allFutures(pending)
     untrackCounter(HttpServerUnsecureConnectionTrackerName)
+    reset(conn[])
     conn.state = HttpState.Closed
 
 proc new(ht: typedesc[HttpConnectionRef], server: HttpServerRef,
@@ -844,7 +846,9 @@ proc closeWait*(req: HttpRequestRef) {.async.} =
           await writer
         except CancelledError:
           await writer
+      reset(resp[])
     untrackCounter(HttpServerRequestTrackerName)
+    reset(req[])
     req.state = HttpState.Closed
 
 proc createConnection(server: HttpServerRef,
@@ -931,6 +935,7 @@ proc getRequestFence*(server: HttpServerRef,
         await connection.getRequest()
       else:
         await connection.getRequest().wait(server.headersTimeout)
+    connection.currentRawQuery = Opt.some(res.rawPath)
     RequestFence.ok(res)
   except CancelledError:
     RequestFence.err(HttpProcessError.init(HttpServerError.InterruptError))
@@ -962,13 +967,17 @@ proc getConnectionFence*(server: HttpServerRef,
     let res = await server.createConnCallback(server, transp)
     ConnectionFence.ok(res)
   except CancelledError:
-    await transp.closeWait()
     ConnectionFence.err(HttpProcessError.init(HttpServerError.InterruptError))
   except HttpCriticalError as exc:
-    await transp.closeWait()
+    # On error `transp` will be closed by `createConnCallback()` call.
-    let address = transp.getRemoteAddress()
+    let address = Opt.none(TransportAddress)
     ConnectionFence.err(HttpProcessError.init(
       HttpServerError.CriticalError, exc, address, exc.code))
+  except CatchableError as exc:
+    # On error `transp` will be closed by `createConnCallback()` call.
+    let address = Opt.none(TransportAddress)
+    ConnectionFence.err(HttpProcessError.init(
+      HttpServerError.CriticalError, exc, address, Http503))
 
 proc processRequest(server: HttpServerRef,
                     connection: HttpConnectionRef,
@@ -984,19 +993,23 @@ proc processRequest(server: HttpServerRef,
     else:
       discard
 
-  defer:
-    if requestFence.isOk():
-      await requestFence.get().closeWait()
-
   let responseFence = await getResponseFence(connection, requestFence)
   if responseFence.isErr() and
      (responseFence.error.kind == HttpServerError.InterruptError):
+    if requestFence.isOk():
+      await requestFence.get().closeWait()
     return HttpProcessExitType.Immediate
 
-  if responseFence.isErr():
+  let res =
-    await connection.sendErrorResponse(requestFence, responseFence.error)
+    if responseFence.isErr():
-  else:
+      await connection.sendErrorResponse(requestFence, responseFence.error)
-    await connection.sendDefaultResponse(requestFence, responseFence.get())
+    else:
+      await connection.sendDefaultResponse(requestFence, responseFence.get())
+
+  if requestFence.isOk():
+    await requestFence.get().closeWait()
+
+  res
 
 proc processLoop(holder: HttpConnectionHolderRef) {.async.} =
   let
@@ -1016,23 +1029,27 @@ proc processLoop(holder: HttpConnectionHolderRef) {.async.} =
   holder.connection = connection
 
   var runLoop = HttpProcessExitType.KeepAlive
-
-  defer:
-    server.connections.del(connectionId)
-    case runLoop
-    of HttpProcessExitType.KeepAlive:
-      # This could happened only on CancelledError.
-      await connection.closeWait()
-    of HttpProcessExitType.Immediate:
-      await connection.closeWait()
-    of HttpProcessExitType.Graceful:
-      await connection.gracefulCloseWait()
-
   while runLoop == HttpProcessExitType.KeepAlive:
-    runLoop = await server.processRequest(connection, connectionId)
+    runLoop =
+      try:
+        await server.processRequest(connection, connectionId)
+      except CancelledError:
+        HttpProcessExitType.Immediate
+      except CatchableError as exc:
+        raiseAssert "Unexpected error [" & $exc.name & "] happens: " & $exc.msg
+
+  server.connections.del(connectionId)
+  case runLoop
+  of HttpProcessExitType.KeepAlive:
+    await connection.closeWait()
+  of HttpProcessExitType.Immediate:
+    await connection.closeWait()
+  of HttpProcessExitType.Graceful:
+    await connection.gracefulCloseWait()
 
 proc acceptClientLoop(server: HttpServerRef) {.async.} =
-  while true:
+  var runLoop = true
+  while runLoop:
     try:
       # if server.maxConnections > 0:
       #   await server.semaphore.acquire()
@@ -1042,27 +1059,18 @@ proc acceptClientLoop(server: HttpServerRef) {.async.} =
         # We are unable to identify remote peer, it means that remote peer
         # disconnected before identification.
         await transp.closeWait()
-        break
+        runLoop = false
       else:
         let connId = resId.get()
         let holder = HttpConnectionHolderRef.new(server, transp, resId.get())
         server.connections[connId] = holder
         holder.future = processLoop(holder)
-    except CancelledError:
+    except TransportTooManyError, TransportAbortedError:
-      # Server was stopped
+      # Non-critical error
-      break
-    except TransportOsError:
-      # This is some critical unrecoverable error.
-      break
-    except TransportTooManyError:
-      # Non critical error
       discard
-    except TransportAbortedError:
+    except CancelledError, TransportOsError, CatchableError:
-      # Non critical error
+      # Critical, cancellation or unexpected error
-      discard
+      runLoop = false
-    except CatchableError:
-      # Unexpected error
-      break
 
 proc state*(server: HttpServerRef): HttpServerState {.raises: [].} =
   ## Returns current HTTP server's state.

chronos/apps/http/httptable.nim

@@ -197,3 +197,7 @@ proc toList*(ht: HttpTables, normKey = false): auto =
   for key, value in ht.stringItems(normKey):
     res.add((key, value))
   res
+
+proc clear*(ht: var HttpTables) =
+  ## Resets the HtppTable so that it is empty.
+  ht.table.clear()

chronos/apps/http/shttpserver.nim

@@ -43,6 +43,7 @@ proc closeSecConnection(conn: HttpConnectionRef) {.async.} =
       await allFutures(pending)
     except CancelledError:
       await allFutures(pending)
+    reset(cast[SecureHttpConnectionRef](conn)[])
     untrackCounter(HttpServerSecureConnectionTrackerName)
     conn.state = HttpState.Closed
 
@@ -74,9 +75,16 @@ proc createSecConnection(server: HttpServerRef,
   except CancelledError as exc:
     await HttpConnectionRef(sconn).closeWait()
     raise exc
-  except TLSStreamError:
+  except TLSStreamError as exc:
     await HttpConnectionRef(sconn).closeWait()
-    raiseHttpCriticalError("Unable to establish secure connection")
+    let msg = "Unable to establish secure connection, reason [" &
+              $exc.msg & "]"
+    raiseHttpCriticalError(msg)
+  except CatchableError as exc:
+    await HttpConnectionRef(sconn).closeWait()
+    let msg = "Unexpected error while trying to establish secure connection, " &
+              "reason [" & $exc.msg & "]"
+    raiseHttpCriticalError(msg)
 
 proc new*(htype: typedesc[SecureHttpServerRef],
           address: TransportAddress,

tests/testhttpserver.nim

@@ -7,9 +7,8 @@
 #              MIT license (LICENSE-MIT)
 import std/[strutils, algorithm]
 import ".."/chronos/unittest2/asynctests,
-       ".."/chronos, ".."/chronos/apps/http/httpserver,
+       ".."/chronos,
-       ".."/chronos/apps/http/httpcommon,
+       ".."/chronos/apps/http/[httpserver, httpcommon, httpdebug]
-       ".."/chronos/apps/http/httpdebug
 import stew/base10
 
 {.used.}
@@ -1357,7 +1356,7 @@ suite "HTTP server testing suite":
   asyncTest "HTTP debug tests":
     const
       TestsCount = 10
-      TestRequest = "GET / HTTP/1.1\r\nConnection: keep-alive\r\n\r\n"
+      TestRequest = "GET /httpdebug HTTP/1.1\r\nConnection: keep-alive\r\n\r\n"
 
     proc process(r: RequestFence): Future[HttpResponseRef] {.async.} =
       if r.isOk():
@@ -1417,6 +1416,7 @@ suite "HTTP server testing suite":
           connection.localAddress.get() == transp.remoteAddress()
           connection.connectionType == ConnectionType.NonSecure
           connection.connectionState == ConnectionState.Alive
+          connection.query.get("") == "/httpdebug"
           (currentTime - connection.createMoment.get()) != ZeroDuration
           (currentTime - connection.acceptMoment) != ZeroDuration
       var pending: seq[Future[void]]

tests/testshttpserver.nim

@@ -7,7 +7,8 @@
 #              MIT license (LICENSE-MIT)
 import std/strutils
 import ".."/chronos/unittest2/asynctests
-import ".."/chronos, ".."/chronos/apps/http/shttpserver
+import ".."/chronos,
+       ".."/chronos/apps/http/shttpserver
 import stew/base10
 
 {.used.}