From 7c80b758566950950ab5e3b29fe7f38c3b8168b0 Mon Sep 17 00:00:00 2001 From: Jacek Sieka Date: Thu, 15 Sep 2022 21:32:16 +0200 Subject: [PATCH] fix enum parsing, work around potential `nil` dereference (#150) * fix enum parsing, work around potential `nil` dereference When the bizarre error handling in the http client fails, it may happen that a nil is returned (maybe due to Nim bugs) - do it manually for now --- json_rpc/client.nim | 5 +++- json_rpc/clients/httpclient.nim | 48 +++++++++++++++++++-------------- json_rpc/jsonmarshal.nim | 10 +++++-- json_rpc/router.nim | 2 +- tests/testrpcmacro.nim | 15 +++++++++++ 5 files changed, 56 insertions(+), 24 deletions(-) diff --git a/json_rpc/client.nim b/json_rpc/client.nim index 54246ff..3ef5568 100644 --- a/json_rpc/client.nim +++ b/json_rpc/client.nim @@ -160,7 +160,10 @@ proc createRpcFromSig*(clientType, rpcDecl: NimNode): NimNode = else: # native json expected so no work callBody.add quote do: - `procRes` = `rpcResult` + `procRes` = if `rpcResult` == nil: + newJNull() + else: + `rpcResult` when defined(nimDumpRpcs): echo pathStr, ":\n", result.repr diff --git a/json_rpc/clients/httpclient.nim b/json_rpc/clients/httpclient.nim index 34fd882..1378fc1 100644 --- a/json_rpc/clients/httpclient.nim +++ b/json_rpc/clients/httpclient.nim @@ -69,22 +69,19 @@ method call*(client: RpcHttpClient, name: string, var req: HttpClientRequestRef var res: HttpClientResponseRef - defer: - # BEWARE! - # Using multiple defer statements in this function or multiple - # try/except blocks within a single defer statement doesn't - # produce the desired run-time code, so we use slightly bizzare - # code to ensure the exceptions safety of this function: - try: - var closeFutures = newSeq[Future[void]]() - if req != nil: closeFutures.add req.closeWait() - if res != nil: closeFutures.add res.closeWait() - if closeFutures.len > 0: await allFutures(closeFutures) - except CatchableError as err: - # TODO - # `close` functions shouldn't raise in general, but we first - # need to ensure this through exception tracking in Chronos - debug "Error closing JSON-RPC HTTP resuest/response", err = err.msg + template closeRefs() = + # We can't trust try/finally in async/await in all nim versions, so we + # do it manually instead + if req != nil: + try: + await req.closeWait() + except CatchableError as exc: # shouldn't happen + debug "Error closing JSON-RPC HTTP resuest/response", err = exc.msg + if res != nil: + try: + await res.closeWait() + except CatchableError as exc: # shouldn't happen + debug "Error closing JSON-RPC HTTP resuest/response", err = exc.msg req = HttpClientRequestRef.post(client.httpSession, client.httpAddress.get, @@ -94,11 +91,14 @@ method call*(client: RpcHttpClient, name: string, try: await req.send() except CancelledError as e: + closeRefs() raise e except CatchableError as e: - raise (ref RpcPostError)(msg: "Failed to send POST Request with JSON-RPC.", parent: e) + closeRefs() + raise (ref RpcPostError)(msg: "Failed to send POST Request with JSON-RPC: " & e.msg, parent: e) if res.status < 200 or res.status >= 300: # res.status is not 2xx (success) + closeRefs() raise newException(ErrorResponse, "POST Response: " & $res.status) debug "Message sent to RPC server", @@ -109,9 +109,11 @@ method call*(client: RpcHttpClient, name: string, try: await res.getBodyBytes(client.maxBodySize) except CancelledError as e: + closeRefs() raise e - except CatchableError as exc: - raise (ref FailedHttpResponse)(msg: "Failed to read POST Response for JSON-RPC.", parent: exc) + except CatchableError as e: + closeRefs() + raise (ref FailedHttpResponse)(msg: "Failed to read POST Response for JSON-RPC: " & e.msg, parent: e) let resText = string.fromBytes(resBytes) trace "Response", text = resText @@ -125,9 +127,15 @@ method call*(client: RpcHttpClient, name: string, try: # Might raise for all kinds of reasons client.processMessage(resText) - finally: + except CatchableError as e: # Need to clean up in case the answer was invalid client.awaiting.del(id) + closeRefs() + raise e + + client.awaiting.del(id) + + closeRefs() # processMessage should have completed this future - if it didn't, `read` will # raise, which is reasonable diff --git a/json_rpc/jsonmarshal.nim b/json_rpc/jsonmarshal.nim index 28a7cbb..12b2a01 100644 --- a/json_rpc/jsonmarshal.nim +++ b/json_rpc/jsonmarshal.nim @@ -1,6 +1,6 @@ import std/[macros, json, options, typetraits], - stew/byteutils + stew/[byteutils, objects] export json, options @@ -33,7 +33,11 @@ proc fromJson*[T](n: JsonNode, argName: string, result: var Option[T]) # This can't be forward declared: https://github.com/nim-lang/Nim/issues/7868 proc fromJson*[T: enum](n: JsonNode, argName: string, result: var T) = n.kind.expect(JInt, argName) - result = n.getBiggestInt().T + + let v = n.getBiggestInt() + if not checkedEnumAssign(result, v): + raise (ref ValueError)( + msg: "Unknown enum ordinal for " & name(T) & ": " & $v) # This can't be forward declared: https://github.com/nim-lang/Nim/issues/7868 proc fromJson*[T: object|tuple](n: JsonNode, argName: string, result: var T) = @@ -145,6 +149,8 @@ proc fromJson*[N, T](n: JsonNode, argName: string, result: var array[N, T]) = proc unpackArg[T](args: JsonNode, argName: string, argtype: typedesc[T]): T = mixin fromJson + if args == nil: + raise (ref ValueError)(msg: argName & ": unexpected null value") fromJson(args, argName, result) proc expectArrayLen(node, jsonIdent: NimNode, length: int) = diff --git a/json_rpc/router.nim b/json_rpc/router.nim index fc699cf..de4cec8 100644 --- a/json_rpc/router.nim +++ b/json_rpc/router.nim @@ -174,4 +174,4 @@ macro rpc*(server: RpcRouter, path: string, body: untyped): untyped = `server`.register(`path`, `rpcProcWrapper`) when defined(nimDumpRpcs): - echo "\n", pathStr, ": ", result.repr + echo "\n", path, ": ", result.repr diff --git a/tests/testrpcmacro.nim b/tests/testrpcmacro.nim index b77c625..051b242 100644 --- a/tests/testrpcmacro.nim +++ b/tests/testrpcmacro.nim @@ -22,6 +22,10 @@ type MyOptionalNotBuiltin = object val: Option[Test2] + MyEnum = enum + Enum0 + Enum1 + let testObj = %*{ "a": %1, @@ -40,6 +44,9 @@ var s = newRpcSocketServer(["localhost:8545"]) s.rpc("rpc.simplePath"): return %1 +s.rpc("rpc.enumParam") do(e: MyEnum): + return %[$e] + s.rpc("rpc.differentParams") do(a: int, b: string): return %[%a, %b] @@ -146,6 +153,14 @@ suite "Server types": let r = waitFor s.executeMethod("rpc.simplePath", %[]) check r == "1" + test "Enum param paths": + block: + let r = waitFor s.executeMethod("rpc.enumParam", %[(int64(Enum1))]) + check r == "[\"Enum1\"]" + + expect(ValueError): + discard waitFor s.executeMethod("rpc.enumParam", %[(int64(42))]) + test "Different param types": let inp = %[%1, %"abc"]