From 6c2ea675123ed0bf5c5d76c92ed4985bacd1a9ec Mon Sep 17 00:00:00 2001 From: Eugene Kabanov Date: Wed, 9 Aug 2023 10:57:49 +0300 Subject: [PATCH] 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 | 9 ++++ chronos/apps/http/httpserver.nim | 88 +++++++++++++++++-------------- chronos/apps/http/httptable.nim | 4 ++ chronos/apps/http/shttpserver.nim | 12 ++++- tests/testhttpserver.nim | 8 +-- tests/testshttpserver.nim | 3 +- 6 files changed, 77 insertions(+), 47 deletions(-) diff --git a/chronos/apps/http/httpdebug.nim b/chronos/apps/http/httpdebug.nim index 2f40674..a1dc022 100644 --- a/chronos/apps/http/httpdebug.nim +++ b/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) diff --git a/chronos/apps/http/httpserver.nim b/chronos/apps/http/httpserver.nim index c1e45c0..eafa27c 100644 --- a/chronos/apps/http/httpserver.nim +++ b/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() - let address = transp.getRemoteAddress() + # On error `transp` will be closed by `createConnCallback()` call. + 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(): - await connection.sendErrorResponse(requestFence, responseFence.error) - else: - await connection.sendDefaultResponse(requestFence, responseFence.get()) + let res = + if responseFence.isErr(): + await connection.sendErrorResponse(requestFence, responseFence.error) + 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: - # Server was stopped - break - except TransportOsError: - # This is some critical unrecoverable error. - break - except TransportTooManyError: - # Non critical error + except TransportTooManyError, TransportAbortedError: + # Non-critical error discard - except TransportAbortedError: - # Non critical error - discard - except CatchableError: - # Unexpected error - break + except CancelledError, TransportOsError, CatchableError: + # Critical, cancellation or unexpected error + runLoop = false proc state*(server: HttpServerRef): HttpServerState {.raises: [].} = ## Returns current HTTP server's state. diff --git a/chronos/apps/http/httptable.nim b/chronos/apps/http/httptable.nim index 86060de..f44765a 100644 --- a/chronos/apps/http/httptable.nim +++ b/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() diff --git a/chronos/apps/http/shttpserver.nim b/chronos/apps/http/shttpserver.nim index b993cb5..bc5c3fb 100644 --- a/chronos/apps/http/shttpserver.nim +++ b/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, diff --git a/tests/testhttpserver.nim b/tests/testhttpserver.nim index 83372ea..0ecc9aa 100644 --- a/tests/testhttpserver.nim +++ b/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/apps/http/httpcommon, - ".."/chronos/apps/http/httpdebug + ".."/chronos, + ".."/chronos/apps/http/[httpserver, httpcommon, 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]] diff --git a/tests/testshttpserver.nim b/tests/testshttpserver.nim index a83d0b2..8aacb8e 100644 --- a/tests/testshttpserver.nim +++ b/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.}