Various error handling and processing fixes (#228)

* remove redundant gcsafe/raises * rework async raises to chronos 4.0 where this was not yet done * streamline logging between http/socket/ws * don't log error when raising exceptions (whoever handles should log) * debug-log requests in all variants of server and client * unify ipv4/ipv6 address resolution, with preference for ipv6 * fix server start so that it consistently raises only when no addresses could be bound
status-im · Oct 22, 2024 · 98a5efb · 98a5efb
1 parent 0408795
commit 98a5efb
Show file tree

Hide file tree

Showing 23 changed files with 477 additions and 553 deletions.
diff --git a/json_rpc.nimble b/json_rpc.nimble
@@ -57,4 +57,4 @@ task test, "run tests":
 
   when not defined(windows):
     # on windows, socker server build failed
-    buildOnly "-d:\"chronicles_sinks=textlines[dynamic],json[dynamic]\"", "tests/all"
+    buildOnly "-d:chronicles_log_level=TRACE -d:\"chronicles_sinks=textlines[dynamic],json[dynamic]\"", "tests/all"
diff --git a/json_rpc/client.nim b/json_rpc/client.nim
@@ -7,8 +7,11 @@
 # This file may not be copied, modified, or distributed except according to
 # those terms.
 
+{.push raises: [], gcsafe.}
+
 import
   std/[json, tables, macros],
+  chronicles,
   chronos,
   results,
   ./jsonmarshal,
@@ -53,8 +56,6 @@ type
 
   GetJsonRpcRequestHeaders* = proc(): seq[(string, string)] {.gcsafe, raises: [].}
 
-{.push gcsafe, raises: [].}
-
 # ------------------------------------------------------------------------------
 # Public helpers
 # ------------------------------------------------------------------------------
@@ -128,22 +129,21 @@ proc getNextId*(client: RpcClient): RequestId =
 
 method call*(client: RpcClient, name: string,
              params: RequestParamsTx): Future[JsonString]
-                {.base, gcsafe, async.} =
-  doAssert(false, "`RpcClient.call` not implemented")
+                {.base, async.} =
+  raiseAssert("`RpcClient.call` not implemented")
 
-method call*(client: RpcClient, name: string,
+proc call*(client: RpcClient, name: string,
              params: JsonNode): Future[JsonString]
-               {.base, gcsafe, async.} =
+               {.async: (raw: true).} =
+  client.call(name, params.paramsTx)
 
-  await client.call(name, params.paramsTx)
-
-method close*(client: RpcClient): Future[void] {.base, gcsafe, async.} =
-  doAssert(false, "`RpcClient.close` not implemented")
+method close*(client: RpcClient): Future[void] {.base, async.} =
+  raiseAssert("`RpcClient.close` not implemented")
 
 method callBatch*(client: RpcClient,
                   calls: RequestBatchTx): Future[ResponseBatchRx]
-                    {.base, gcsafe, async.} =
-  doAssert(false, "`RpcClient.callBatch` not implemented")
+                    {.base, async.} =
+  raiseAssert("`RpcClient.callBatch` not implemented")
 
 proc processMessage*(client: RpcClient, line: string): Result[void, string] =
   if client.onProcessMessage.isNil.not:
@@ -152,8 +152,6 @@ proc processMessage*(client: RpcClient, line: string): Result[void, string] =
     if not fallBack:
       return ok()
 
-  # Note: this doesn't use any transport code so doesn't need to be
-  # differentiated.
   try:
     let batch = JrpcSys.decode(line, ResponseBatchRx)
     if batch.kind == rbkMany:
@@ -166,15 +164,14 @@ proc processMessage*(client: RpcClient, line: string): Result[void, string] =
     if response.jsonrpc.isNone:
       return err("missing or invalid `jsonrpc`")
 
-    if response.id.isNone:
+    let id = response.id.valueOr:
       if response.error.isSome:
         let error = JrpcSys.encode(response.error.get)
         return err(error)
       else:
         return err("missing or invalid response id")
 
     var requestFut: Future[JsonString]
-    let id = response.id.get
     if not client.awaiting.pop(id, requestFut):
       return err("Cannot find message id \"" & $id & "\"")
 
@@ -189,6 +186,8 @@ proc processMessage*(client: RpcClient, line: string): Result[void, string] =
       requestFut.fail(newException(JsonRpcError, msg))
       return ok()
 
+    debug "Received JSON-RPC response",
+      len = string(response.result).len, id
     requestFut.complete(response.result)
     return ok()
 

diff --git a/json_rpc/clients/httpclient.nim b/json_rpc/clients/httpclient.nim
@@ -7,6 +7,8 @@
 # This file may not be copied, modified, or distributed except according to
 # those terms.
 
+{.push raises: [], gcsafe.}
+
 import
   std/[tables, uri],
   stew/byteutils,
@@ -33,12 +35,13 @@ type
     maxBodySize: int
     getHeaders: GetJsonRpcRequestHeaders
 
-{.push gcsafe, raises: [].}
-
 # ------------------------------------------------------------------------------
 # Private helpers
 # ------------------------------------------------------------------------------
 
+proc `$`(v: HttpAddress): string =
+  v.id
+
 proc new(
     T: type RpcHttpClient, maxBodySize = MaxMessageBodyBytes, secure = false,
     getHeaders: GetJsonRpcRequestHeaders = nil, flags: HttpClientFlags = {}): T =
@@ -136,14 +139,14 @@ proc newRpcHttpClient*(
 
 method call*(client: RpcHttpClient, name: string,
              params: RequestParamsTx): Future[JsonString]
-            {.async, gcsafe.} =
+            {.async.} =
 
   let
     id = client.getNextId()
     reqBody = requestTxEncode(name, params, id)
 
-  debug "Sending message to RPC server",
-         address = client.httpAddress, msg_len = len(reqBody), name
+  debug "Sending JSON-RPC request",
+         address = client.httpAddress, len = len(reqBody), name, id
   trace "Message", msg = reqBody
 
   let resText = await client.callImpl(reqBody)
@@ -158,7 +161,6 @@ method call*(client: RpcHttpClient, name: string,
   let msgRes = client.processMessage(resText)
   if msgRes.isErr:
     # Need to clean up in case the answer was invalid
-    error "Failed to process POST Response for JSON-RPC", msg = msgRes.error
     let exc = newException(JsonRpcError, msgRes.error)
     newFut.fail(exc)
     client.awaiting.del(id)
@@ -177,10 +179,11 @@ method call*(client: RpcHttpClient, name: string,
 
 method callBatch*(client: RpcHttpClient,
                   calls: RequestBatchTx): Future[ResponseBatchRx]
-                    {.gcsafe, async.} =
-  let
-    reqBody = requestBatchEncode(calls)
-    resText = await client.callImpl(reqBody)
+                    {.async.} =
+  let reqBody = requestBatchEncode(calls)
+  debug "Sending JSON-RPC batch",
+        address = $client.httpAddress, len = len(reqBody)
+  let resText = await client.callImpl(reqBody)
 
   if client.batchFut.isNil or client.batchFut.finished():
     client.batchFut = newFuture[ResponseBatchRx]()

diff --git a/json_rpc/clients/socketclient.nim b/json_rpc/clients/socketclient.nim
@@ -7,6 +7,8 @@
 # This file may not be copied, modified, or distributed except according to
 # those terms.
 
+{.push raises: [], gcsafe.}
+
 import
   std/tables,
   chronicles,
@@ -29,8 +31,6 @@ type
 
 const defaultMaxRequestLength* = 1024 * 128
 
-{.push gcsafe, raises: [].}
-
 proc new*(T: type RpcSocketClient): T =
   T()
 
@@ -39,41 +39,46 @@ proc newRpcSocketClient*: RpcSocketClient =
   RpcSocketClient.new()
 
 method call*(client: RpcSocketClient, name: string,
-             params: RequestParamsTx): Future[JsonString] {.async, gcsafe.} =
+             params: RequestParamsTx): Future[JsonString] {.async.} =
   ## Remotely calls the specified RPC method.
-  let id = client.getNextId()
-  var jsonBytes = requestTxEncode(name, params, id) & "\r\n"
   if client.transport.isNil:
     raise newException(JsonRpcError,
                     "Transport is not initialised (missing a call to connect?)")
 
-  # completed by processMessage.
-  var newFut = newFuture[JsonString]()
+  let
+    id = client.getNextId()
+    reqBody = requestTxEncode(name, params, id) & "\r\n"
+    newFut = newFuture[JsonString]()  # completed by processMessage
+
   # add to awaiting responses
   client.awaiting[id] = newFut
 
-  let res = await client.transport.write(jsonBytes)
+  debug "Sending JSON-RPC request",
+         address = $client.address, len = len(reqBody), name, id
+
+  let res = await client.transport.write(reqBody)
   # TODO: Add actions when not full packet was send, e.g. disconnect peer.
-  doAssert(res == jsonBytes.len)
+  doAssert(res == reqBody.len)
 
   return await newFut
 
 method callBatch*(client: RpcSocketClient,
                   calls: RequestBatchTx): Future[ResponseBatchRx]
-                    {.gcsafe, async.} =
+                    {.async.} =
   if client.transport.isNil:
     raise newException(JsonRpcError,
       "Transport is not initialised (missing a call to connect?)")
 
   if client.batchFut.isNil or client.batchFut.finished():
     client.batchFut = newFuture[ResponseBatchRx]()
 
-  let
-    jsonBytes = requestBatchEncode(calls) & "\r\n"
-    res = await client.transport.write(jsonBytes)
+  let reqBody = requestBatchEncode(calls) & "\r\n"
+  debug "Sending JSON-RPC batch",
+        address = $client.address, len = len(reqBody)
+  let res = await client.transport.write(reqBody)
 
   # TODO: Add actions when not full packet was send, e.g. disconnect peer.
-  doAssert(res == jsonBytes.len)
+  doAssert(res == reqBody.len)
 
   return await client.batchFut
 
@@ -90,7 +95,6 @@ proc processData(client: RpcSocketClient) {.async: (raises: []).} =
 
         let res = client.processMessage(value)
         if res.isErr:
-          error "Error when processing RPC message", msg=res.error
           localException = newException(JsonRpcError, res.error)
           break
       except TransportError as exc:
@@ -116,7 +120,7 @@ proc processData(client: RpcSocketClient) {.async: (raises: []).} =
       error "Error when reconnecting to server", msg=exc.msg
       break
     except CancelledError as exc:
-      error "Error when reconnecting to server", msg=exc.msg
+      debug "Server connection was cancelled", msg=exc.msg
       break
 
 proc connect*(client: RpcSocketClient, address: string, port: Port) {.async.} =

diff --git a/json_rpc/clients/websocketclient.nim b/json_rpc/clients/websocketclient.nim
@@ -7,6 +7,8 @@
 # This file may not be copied, modified, or distributed except according to
 # those terms.
 
+{.push raises: [], gcsafe.}
+
 import
   ./websocketclientimpl,
   ../client

diff --git a/json_rpc/clients/websocketclientimpl.nim b/json_rpc/clients/websocketclientimpl.nim
@@ -7,8 +7,10 @@
 # This file may not be copied, modified, or distributed except according to
 # those terms.
 
+{.push raises: [], gcsafe.}
+
 import
-  std/[uri, strutils],
+  std/uri,
   pkg/websock/[websock, extensions/compression/deflate],
   pkg/[chronos, chronos/apps/http/httptable, chronicles],
   stew/byteutils,
@@ -27,8 +29,6 @@ type
     loop*: Future[void]
     getHeaders*: GetJsonRpcRequestHeaders
 
-{.push gcsafe, raises: [].}
-
 proc new*(
     T: type RpcWebSocketClient, getHeaders: GetJsonRpcRequestHeaders = nil): T =
   T(getHeaders: getHeaders)
@@ -39,35 +39,39 @@ proc newRpcWebSocketClient*(
   RpcWebSocketClient.new(getHeaders)
 
 method call*(client: RpcWebSocketClient, name: string,
-             params: RequestParamsTx): Future[JsonString] {.async, gcsafe.} =
+             params: RequestParamsTx): Future[JsonString] {.async.} =
   ## Remotely calls the specified RPC method.
   if client.transport.isNil:
     raise newException(JsonRpcError,
       "Transport is not initialised (missing a call to connect?)")
 
-  let id = client.getNextId()
-  var value = requestTxEncode(name, params, id) & "\r\n"
-
-  # completed by processMessage.
-  var newFut = newFuture[JsonString]()
+  let
+    id = client.getNextId()
+    reqBody = requestTxEncode(name, params, id) & "\r\n"
+    newFut = newFuture[JsonString]() # completed by processMessage
   # add to awaiting responses
   client.awaiting[id] = newFut
 
-  await client.transport.send(value)
+  debug "Sending JSON-RPC request",
+         address = $client.uri, len = len(reqBody), name
+
+  await client.transport.send(reqBody)
   return await newFut
 
 method callBatch*(client: RpcWebSocketClient,
                   calls: RequestBatchTx): Future[ResponseBatchRx]
-                    {.gcsafe, async.} =
+                    {.async.} =
   if client.transport.isNil:
     raise newException(JsonRpcError,
       "Transport is not initialised (missing a call to connect?)")
 
   if client.batchFut.isNil or client.batchFut.finished():
     client.batchFut = newFuture[ResponseBatchRx]()
 
-  let jsonBytes = requestBatchEncode(calls) & "\r\n"
-  await client.transport.send(jsonBytes)
+  let reqBody = requestBatchEncode(calls) & "\r\n"
+  debug "Sending JSON-RPC batch",
+         address = $client.uri, len = len(reqBody)
+  await client.transport.send(reqBody)
 
   return await client.batchFut
 
@@ -92,7 +96,6 @@ proc processData(client: RpcWebSocketClient) {.async.} =
 
       let res = client.processMessage(string.fromBytes(value))
       if res.isErr:
-        error "Error when processing RPC message", msg=res.error
         error = newException(JsonRpcError, res.error)
         processError()
 

diff --git a/json_rpc/errors.nim b/json_rpc/errors.nim
@@ -7,7 +7,7 @@
 # This file may not be copied, modified, or distributed except according to
 # those terms.
 
-{.push raises: [].}
+{.push raises: [], gcsafe.}
 
 import results, json_serialization
 

diff --git a/json_rpc/jsonmarshal.nim b/json_rpc/jsonmarshal.nim
@@ -7,6 +7,8 @@
 # This file may not be copied, modified, or distributed except according to
 # those terms.
 
+{.push raises: [], gcsafe.}
+
 import
   json_serialization
 
@@ -19,6 +21,6 @@ createJsonFlavor JrpcConv,
   omitOptionalFields = true, # Skip optional fields==none in Writer
   allowUnknownFields = true,
   skipNullFields = true      # Skip optional fields==null in Reader
-                           
+
 # JrpcConv is a namespace/flavor for encoding and decoding
 # parameters and return value of a rpc method.