From 3803d5cdea6940ed501c45f6fe24817c3bd68ad3 Mon Sep 17 00:00:00 2001 From: Henrique Dias Date: Sun, 6 Jun 2021 12:33:39 +0100 Subject: [PATCH 1/4] Fix for #18161 Too many open files. I've been checking the code in asynchttpserver file and found that in the processClient function the connection is never closed after the "while" loop. So even if the keepalive header is off the number of open files is always increasing. With the keepalive off and if the connection is closed after the "while" loop the server never dies. If the header keepalive is on and if it exceeds the pre-defined number of file descriptors it automatically closes the connection! I also added a timeout for keepalive connections. --- lib/pure/asynchttpserver.nim | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/lib/pure/asynchttpserver.nim b/lib/pure/asynchttpserver.nim index 4500d994eff11..81c1f77b70b11 100644 --- a/lib/pure/asynchttpserver.nim +++ b/lib/pure/asynchttpserver.nim @@ -44,6 +44,7 @@ import asyncnet, asyncdispatch, parseutils, uri, strutils import httpcore +from times import DateTime, now, `-`, inSeconds export httpcore except parseHeader @@ -289,6 +290,9 @@ proc processRequest( request.client.close() return false +const + keepaliveTimeout* {.intdefine.} = 60 ### default value for keepaliveTimeout in seconds + proc processClient(server: AsyncHttpServer, client: AsyncSocket, address: string, callback: proc (request: Request): Future[void] {.closure, gcsafe.}) {.async.} = @@ -298,12 +302,20 @@ proc processClient(server: AsyncHttpServer, client: AsyncSocket, address: string var lineFut = newFutureVar[string]("asynchttpserver.processClient") lineFut.mget() = newStringOfCap(80) + let startTimeout = now() while not client.isClosed: + let fds = activeDescriptors() + # The maxFDs should be replaced by the keepaliveConn? + if (fds > server.maxFDs) or ((now() - startTimeout).inSeconds > keepaliveTimeout): + break # Connection timeout + let retry = await processRequest( server, request, client, address, lineFut, callback ) if not retry: break + client.close() # Close the connection to not increase the number of open files. + const nimMaxDescriptorsFallback* {.intdefine.} = 16_000 ## fallback value for \ ## when `maxDescriptors` is not available. @@ -339,6 +351,7 @@ proc acceptRequest*(server: AsyncHttpServer, var (address, client) = await server.socket.acceptAddr() asyncCheck processClient(server, client, address, callback) + proc serve*(server: AsyncHttpServer, port: Port, callback: proc (request: Request): Future[void] {.closure, gcsafe.}, address = ""; @@ -365,6 +378,7 @@ proc serve*(server: AsyncHttpServer, port: Port, #echo(f.isNil) #echo(f.repr) + proc close*(server: AsyncHttpServer) = ## Terminates the async http server instance. server.socket.close() From 71b93bbf647e3beea91bb61c964018538bfafbd2 Mon Sep 17 00:00:00 2001 From: Henrique Dias Date: Sun, 6 Jun 2021 16:59:03 +0100 Subject: [PATCH 2/4] Store keepaliveTimeout option in AsyncHttpServer The default value for keepalive timeout is 3600 seconds. --- lib/pure/asynchttpserver.nim | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/pure/asynchttpserver.nim b/lib/pure/asynchttpserver.nim index 81c1f77b70b11..f68380ec64836 100644 --- a/lib/pure/asynchttpserver.nim +++ b/lib/pure/asynchttpserver.nim @@ -72,11 +72,14 @@ type reusePort: bool maxBody: int ## The maximum content-length that will be read for the body. maxFDs: int + keepaliveTimeout: int ## The value for keepalive timeout in seconds and \ + ## after which the connection will be closed. proc newAsyncHttpServer*(reuseAddr = true, reusePort = false, - maxBody = 8388608): AsyncHttpServer = + maxBody = 8388608, keepaliveTimeout = 3600): AsyncHttpServer = ## Creates a new ``AsyncHttpServer`` instance. - result = AsyncHttpServer(reuseAddr: reuseAddr, reusePort: reusePort, maxBody: maxBody) + result = AsyncHttpServer(reuseAddr: reuseAddr, reusePort: reusePort, + maxBody: maxBody, keepaliveTimeout: keepaliveTimeout) proc addHeaders(msg: var string, headers: HttpHeaders) = for k, v in headers: @@ -290,9 +293,6 @@ proc processRequest( request.client.close() return false -const - keepaliveTimeout* {.intdefine.} = 60 ### default value for keepaliveTimeout in seconds - proc processClient(server: AsyncHttpServer, client: AsyncSocket, address: string, callback: proc (request: Request): Future[void] {.closure, gcsafe.}) {.async.} = @@ -306,7 +306,7 @@ proc processClient(server: AsyncHttpServer, client: AsyncSocket, address: string while not client.isClosed: let fds = activeDescriptors() # The maxFDs should be replaced by the keepaliveConn? - if (fds > server.maxFDs) or ((now() - startTimeout).inSeconds > keepaliveTimeout): + if (fds > server.maxFDs) or ((now() - startTimeout).inSeconds > server.keepaliveTimeout): break # Connection timeout let retry = await processRequest( From ffbd7dd47fd89a205c82dfc201430dd32bbb016b Mon Sep 17 00:00:00 2001 From: Henrique Dias Date: Sun, 6 Jun 2021 17:41:22 +0100 Subject: [PATCH 3/4] Added a test to temporarily disable the keepalive persistent connections. Disables the keepalive if the active file descriptors exceeds the maxFDs value or if the maximum keepalive timeout is exceeded. In this case, the new connections are closed until the value of the file descriptors drops back down to the maxFDs. --- lib/pure/asynchttpserver.nim | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/lib/pure/asynchttpserver.nim b/lib/pure/asynchttpserver.nim index f68380ec64836..0ae5a09d40f93 100644 --- a/lib/pure/asynchttpserver.nim +++ b/lib/pure/asynchttpserver.nim @@ -65,6 +65,7 @@ type url*: Uri hostname*: string ## The hostname of the client that made the request. body*: string + disableKeepalive: bool AsyncHttpServer* = ref object socket: AsyncSocket @@ -280,7 +281,8 @@ proc processRequest( # connection will not be closed and will be kept in the connection pool. # Persistent connections - if (request.protocol == HttpVer11 and + if (not request.disableKeepalive and + request.protocol == HttpVer11 and cmpIgnoreCase(request.headers.getOrDefault("connection"), "close") != 0) or (request.protocol == HttpVer10 and cmpIgnoreCase(request.headers.getOrDefault("connection"), "keep-alive") == 0): @@ -299,15 +301,18 @@ proc processClient(server: AsyncHttpServer, client: AsyncSocket, address: string var request = newFutureVar[Request]("asynchttpserver.processClient") request.mget().url = initUri() request.mget().headers = newHttpHeaders() + request.mget().disableKeepalive = false var lineFut = newFutureVar[string]("asynchttpserver.processClient") lineFut.mget() = newStringOfCap(80) let startTimeout = now() while not client.isClosed: let fds = activeDescriptors() + # Disables the keepalive if the active file descriptors exceeds the maxFDs value + # or if the maximum keepalive timeout is exceeded. # The maxFDs should be replaced by the keepaliveConn? if (fds > server.maxFDs) or ((now() - startTimeout).inSeconds > server.keepaliveTimeout): - break # Connection timeout + request.mget().disableKeepalive = true let retry = await processRequest( server, request, client, address, lineFut, callback @@ -316,6 +321,7 @@ proc processClient(server: AsyncHttpServer, client: AsyncSocket, address: string client.close() # Close the connection to not increase the number of open files. + const nimMaxDescriptorsFallback* {.intdefine.} = 16_000 ## fallback value for \ ## when `maxDescriptors` is not available. From 172f15e353331ef8ac7f6d277a72004f19882586 Mon Sep 17 00:00:00 2001 From: Henrique Dias Date: Sun, 6 Jun 2021 21:33:16 +0100 Subject: [PATCH 4/4] Fix a small bug! --- lib/pure/asynchttpserver.nim | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/pure/asynchttpserver.nim b/lib/pure/asynchttpserver.nim index 0ae5a09d40f93..86f12fa24f27e 100644 --- a/lib/pure/asynchttpserver.nim +++ b/lib/pure/asynchttpserver.nim @@ -281,11 +281,11 @@ proc processRequest( # connection will not be closed and will be kept in the connection pool. # Persistent connections - if (not request.disableKeepalive and - request.protocol == HttpVer11 and + if not request.disableKeepalive and + (request.protocol == HttpVer11 and cmpIgnoreCase(request.headers.getOrDefault("connection"), "close") != 0) or (request.protocol == HttpVer10 and - cmpIgnoreCase(request.headers.getOrDefault("connection"), "keep-alive") == 0): + cmpIgnoreCase(request.headers.getOrDefault("connection"), "keep-alive") == 0)): # In HTTP 1.1 we assume that connection is persistent. Unless connection # header states otherwise. # In HTTP 1.0 we assume that the connection should not be persistent.