From e38ceb5378e7ce945eedbe1c6fb670095cfb9cc5 Mon Sep 17 00:00:00 2001 From: Jacek Sieka Date: Mon, 4 Dec 2023 14:19:29 +0100 Subject: [PATCH] fix v3 backwards compatibility for callbacks (#481) Because the callback types were used explicitly in some consumers of chronos, the change of type introduces a backwards incompatibility preventing a smooth transition to v4 for code that doesn't uses `raises`. This PR restores backwards compatibility at the expense of introducing a new type with a potentially ugly name - that said, there is already precedence for using numbered names to provide new error handling strategy in chronos. --- chronos/apps/http/httpserver.nim | 59 +++++++++++++++++++++++++++---- chronos/apps/http/shttpserver.nim | 52 ++++++++++++++++++++++++++- chronos/transports/stream.nim | 23 ++++++------ tests/testhttpclient.nim | 2 +- 4 files changed, 117 insertions(+), 19 deletions(-) diff --git a/chronos/apps/http/httpserver.nim b/chronos/apps/http/httpserver.nim index 7d1aea0..3bbee0f 100644 --- a/chronos/apps/http/httpserver.nim +++ b/chronos/apps/http/httpserver.nim @@ -63,18 +63,22 @@ type HttpResponseState* {.pure.} = enum Empty, Prepared, Sending, Finished, Failed, Cancelled, Default - HttpProcessCallback* = + # TODO Evaluate naming of raises-annotated callbacks + HttpProcessCallback2* = proc(req: RequestFence): Future[HttpResponseRef] {. - async: (raises: [CancelledError, HttpResponseError]), gcsafe.} + async: (raises: [CancelledError, HttpResponseError]).} + + HttpProcessCallback* {.deprecated.} = + proc(req: RequestFence): Future[HttpResponseRef] {.async.} HttpConnectionCallback* = proc(server: HttpServerRef, transp: StreamTransport): Future[HttpConnectionRef] {. - async: (raises: [CancelledError, HttpConnectionError]), gcsafe.} + async: (raises: [CancelledError, HttpConnectionError]).} HttpCloseConnectionCallback* = proc(connection: HttpConnectionRef): Future[void] {. - async: (raises: []), gcsafe.} + async: (raises: []).} HttpConnectionHolder* = object of RootObj connection*: HttpConnectionRef @@ -103,7 +107,7 @@ type bufferSize*: int maxHeadersSize*: int maxRequestBodySize*: int - processCallback*: HttpProcessCallback + processCallback*: HttpProcessCallback2 createConnCallback*: HttpConnectionCallback HttpServerRef* = ref HttpServer @@ -182,7 +186,7 @@ proc createConnection(server: HttpServerRef, proc new*(htype: typedesc[HttpServerRef], address: TransportAddress, - processCallback: HttpProcessCallback, + processCallback: HttpProcessCallback2, serverFlags: set[HttpServerFlags] = {}, socketFlags: set[ServerFlags] = {ReuseAddr}, serverUri = Uri(), @@ -236,6 +240,49 @@ proc new*(htype: typedesc[HttpServerRef], ) ok(res) +proc new*(htype: typedesc[HttpServerRef], + address: TransportAddress, + processCallback: HttpProcessCallback, + serverFlags: set[HttpServerFlags] = {}, + socketFlags: set[ServerFlags] = {ReuseAddr}, + serverUri = Uri(), + serverIdent = "", + maxConnections: int = -1, + bufferSize: int = 4096, + backlogSize: int = DefaultBacklogSize, + httpHeadersTimeout = 10.seconds, + maxHeadersSize: int = 8192, + maxRequestBodySize: int = 1_048_576, + dualstack = DualStackType.Auto): HttpResult[HttpServerRef] {. + deprecated: "raises missing from process callback".} = + + proc processCallback2(req: RequestFence): Future[HttpResponseRef] {. + async: (raises: [CancelledError, HttpResponseError]).} = + try: + await processCallback(req) + except CancelledError as exc: + raise exc + except HttpResponseError as exc: + raise exc + except CatchableError as exc: + # Emulate 3.x behavior + raise (ref HttpCriticalError)(msg: exc.msg, code: Http503) + + HttpServerRef.new( + address = address, + processCallback = processCallback2, + serverFlags = serverFlags, + socketFlags = socketFlags, + serverUri = serverUri, + serverIdent = serverIdent, + maxConnections = maxConnections, + bufferSize = bufferSize, + backlogSize = backlogSize, + httpHeadersTimeout = httpHeadersTimeout, + maxHeadersSize = maxHeadersSize, + maxRequestBodySize = maxRequestBodySize, + dualstack = dualstack) + proc getServerFlags(req: HttpRequestRef): set[HttpServerFlags] = var defaultFlags: set[HttpServerFlags] = {} if isNil(req): return defaultFlags diff --git a/chronos/apps/http/shttpserver.nim b/chronos/apps/http/shttpserver.nim index 2373d95..f7e377f 100644 --- a/chronos/apps/http/shttpserver.nim +++ b/chronos/apps/http/shttpserver.nim @@ -82,7 +82,7 @@ proc createSecConnection(server: HttpServerRef, proc new*(htype: typedesc[SecureHttpServerRef], address: TransportAddress, - processCallback: HttpProcessCallback, + processCallback: HttpProcessCallback2, tlsPrivateKey: TLSPrivateKey, tlsCertificate: TLSCertificate, serverFlags: set[HttpServerFlags] = {}, @@ -145,3 +145,53 @@ proc new*(htype: typedesc[SecureHttpServerRef], secureFlags: secureFlags ) ok(res) + +proc new*(htype: typedesc[SecureHttpServerRef], + address: TransportAddress, + processCallback: HttpProcessCallback, + tlsPrivateKey: TLSPrivateKey, + tlsCertificate: TLSCertificate, + serverFlags: set[HttpServerFlags] = {}, + socketFlags: set[ServerFlags] = {ReuseAddr}, + serverUri = Uri(), + serverIdent = "", + secureFlags: set[TLSFlags] = {}, + maxConnections: int = -1, + bufferSize: int = 4096, + backlogSize: int = DefaultBacklogSize, + httpHeadersTimeout = 10.seconds, + maxHeadersSize: int = 8192, + maxRequestBodySize: int = 1_048_576, + dualstack = DualStackType.Auto + ): HttpResult[SecureHttpServerRef] {. + deprecated: "raises missing from process callback".} = + proc processCallback2(req: RequestFence): Future[HttpResponseRef] {. + async: (raises: [CancelledError, HttpResponseError]).} = + try: + await processCallback(req) + except CancelledError as exc: + raise exc + except HttpResponseError as exc: + raise exc + except CatchableError as exc: + # Emulate 3.x behavior + raise (ref HttpCriticalError)(msg: exc.msg, code: Http503) + + SecureHttpServerRef.new( + address = address, + processCallback = processCallback2, + tlsPrivateKey = tlsPrivateKey, + tlsCertificate = tlsCertificate, + serverFlags = serverFlags, + socketFlags = socketFlags, + serverUri = serverUri, + serverIdent = serverIdent, + secureFlags = secureFlags, + maxConnections = maxConnections, + bufferSize = bufferSize, + backlogSize = backlogSize, + httpHeadersTimeout = httpHeadersTimeout, + maxHeadersSize = maxHeadersSize, + maxRequestBodySize = maxRequestBodySize, + dualstack = dualstack + ) \ No newline at end of file diff --git a/chronos/transports/stream.nim b/chronos/transports/stream.nim index 107bc6e..c0d1cfc 100644 --- a/chronos/transports/stream.nim +++ b/chronos/transports/stream.nim @@ -115,14 +115,15 @@ else: discard type - StreamCallback* = proc(server: StreamServer, - client: StreamTransport) {.async: (raises: []).} + # TODO evaluate naming of raises-annotated callbacks + StreamCallback2* = proc(server: StreamServer, + client: StreamTransport) {.async: (raises: []).} ## New remote client connection callback ## ``server`` - StreamServer object. ## ``client`` - accepted client transport. - UnsafeStreamCallback* = proc(server: StreamServer, - client: StreamTransport) {.async.} + StreamCallback* = proc(server: StreamServer, + client: StreamTransport) {.async.} ## Connection callback that doesn't check for exceptions at compile time ## ``server`` - StreamServer object. ## ``client`` - accepted client transport. @@ -135,7 +136,7 @@ type StreamServer* = ref object of SocketServer ## StreamServer object - function*: StreamCallback # callback which will be called after new + function*: StreamCallback2 # callback which will be called after new # client accepted init*: TransportInitCallback # callback which will be called before # transport for new client @@ -1870,7 +1871,7 @@ proc getBacklogSize(backlog: int): cint = cint(backlog) proc createStreamServer*(host: TransportAddress, - cbproc: StreamCallback, + cbproc: StreamCallback2, flags: set[ServerFlags] = {}, sock: AsyncFD = asyncInvalidSocket, backlog: int = DefaultBacklogSize, @@ -2092,7 +2093,7 @@ proc createStreamServer*(host: TransportAddress, sres proc createStreamServer*(host: TransportAddress, - cbproc: UnsafeStreamCallback, + cbproc: StreamCallback, flags: set[ServerFlags] = {}, sock: AsyncFD = asyncInvalidSocket, backlog: int = DefaultBacklogSize, @@ -2124,11 +2125,11 @@ proc createStreamServer*(host: TransportAddress, udata: pointer = nil, dualstack = DualStackType.Auto): StreamServer {. raises: [TransportOsError].} = - createStreamServer(host, StreamCallback(nil), flags, sock, backlog, bufferSize, + createStreamServer(host, StreamCallback2(nil), flags, sock, backlog, bufferSize, child, init, cast[pointer](udata), dualstack) proc createStreamServer*[T](host: TransportAddress, - cbproc: StreamCallback, + cbproc: StreamCallback2, flags: set[ServerFlags] = {}, udata: ref T, sock: AsyncFD = asyncInvalidSocket, @@ -2144,7 +2145,7 @@ proc createStreamServer*[T](host: TransportAddress, child, init, cast[pointer](udata), dualstack) proc createStreamServer*[T](host: TransportAddress, - cbproc: UnsafeStreamCallback, + cbproc: StreamCallback, flags: set[ServerFlags] = {}, udata: ref T, sock: AsyncFD = asyncInvalidSocket, @@ -2172,7 +2173,7 @@ proc createStreamServer*[T](host: TransportAddress, raises: [TransportOsError].} = var fflags = flags + {GCUserData} GC_ref(udata) - createStreamServer(host, StreamCallback(nil), fflags, sock, backlog, bufferSize, + createStreamServer(host, StreamCallback2(nil), fflags, sock, backlog, bufferSize, child, init, cast[pointer](udata), dualstack) proc getUserData*[T](server: StreamServer): T {.inline.} = diff --git a/tests/testhttpclient.nim b/tests/testhttpclient.nim index eb1eaac..d2a355d 100644 --- a/tests/testhttpclient.nim +++ b/tests/testhttpclient.nim @@ -85,7 +85,7 @@ suite "HTTP client testing suite": res proc createServer(address: TransportAddress, - process: HttpProcessCallback, secure: bool): HttpServerRef = + process: HttpProcessCallback2, secure: bool): HttpServerRef = let socketFlags = {ServerFlags.TcpNoDelay, ServerFlags.ReuseAddr} serverFlags = {HttpServerFlags.Http11Pipeline}