fix: code review

Author: richard-ramos

benchmarks/bench_client.nim

@@ -109,7 +109,7 @@ proc runLatencyStream(conn: Connection, runs: int): Future[StreamResult] {.async
 proc modeThroughput(
     serverAddr: TransportAddress, uploadSize, downloadSize, chunkSize, runs: int
 ): Future[RunResult] {.async.} =
-  var result = RunResult(
+  var runResult = RunResult(
     mode: Throughput,
     connections: 1,
     streamsPerConn: 1,
@@ -128,17 +128,17 @@ proc modeThroughput(
     connRes.streamResults.add(sr)
 
   connRes.durationNs = (Moment.now() - start).nanoseconds
-  result.connResults.add(connRes)
-  result.durationNs = connRes.durationNs
+  runResult.connResults.add(connRes)
+  runResult.durationNs = connRes.durationNs
 
   conn.close()
   await client.stop()
-  return result
+  return runResult
 
 # -- Mode: latency (1 conn, 1 stream) --
 
 proc modeLatency(serverAddr: TransportAddress, runs: int): Future[RunResult] {.async.} =
-  var result = RunResult(mode: Latency, connections: 1, streamsPerConn: 1)
+  var runResult = RunResult(mode: Latency, connections: 1, streamsPerConn: 1)
 
   let client = makeClient()
   let conn = await client.dial(serverAddr)
@@ -148,20 +148,20 @@ proc modeLatency(serverAddr: TransportAddress, runs: int): Future[RunResult] {.a
   let sr = await runLatencyStream(conn, runs)
   connRes.streamResults.add(sr)
   connRes.durationNs = (Moment.now() - start).nanoseconds
-  result.connResults.add(connRes)
-  result.durationNs = connRes.durationNs
+  runResult.connResults.add(connRes)
+  runResult.durationNs = connRes.durationNs
 
   conn.close()
   await client.stop()
-  return result
+  return runResult
 
 # -- Mode: multistream (1 conn, K streams) --
 
 proc modeMultiStream(
     serverAddr: TransportAddress,
     numStreams, uploadSize, downloadSize, chunkSize, runs: int,
 ): Future[RunResult] {.async.} =
-  var result = RunResult(
+  var runResult = RunResult(
     mode: MultiStream,
     connections: 1,
     streamsPerConn: numStreams,
@@ -191,20 +191,20 @@ proc modeMultiStream(
       connRes.streamResults.add(sr)
 
   connRes.durationNs = (Moment.now() - start).nanoseconds
-  result.connResults.add(connRes)
-  result.durationNs = connRes.durationNs
+  runResult.connResults.add(connRes)
+  runResult.durationNs = connRes.durationNs
 
   conn.close()
   await client.stop()
-  return result
+  return runResult
 
 # -- Mode: multiconn (N conns, 1 stream each) --
 
 proc modeMultiConn(
     serverAddr: TransportAddress,
     numConns, uploadSize, downloadSize, chunkSize, runs: int,
 ): Future[RunResult] {.async.} =
-  var result = RunResult(
+  var runResult = RunResult(
     mode: MultiConn,
     connections: numConns,
     streamsPerConn: 1,
@@ -235,30 +235,30 @@ proc modeMultiConn(
 
     for i, f in futs:
       let sr = await f
-      # Associate with the right connection result
-      while result.connResults.len <= i:
-        result.connResults.add(ConnectionResult())
-      result.connResults[i].streamResults.add(sr)
+      # Associate with the right connection metrics
+      while runResult.connResults.len <= i:
+        runResult.connResults.add(ConnectionResult())
+      runResult.connResults[i].streamResults.add(sr)
 
   let totalDur = (Moment.now() - start).nanoseconds
-  for cr in result.connResults.mitems:
+  for cr in runResult.connResults.mitems:
     cr.durationNs = totalDur
-  result.durationNs = totalDur
+  runResult.durationNs = totalDur
 
   for conn in conns:
     conn.close()
   for client in clients:
     await client.stop()
 
-  return result
+  return runResult
 
 # -- Mode: stress (N conns, K streams each) --
 
 proc modeStress(
     serverAddr: TransportAddress,
     numConns, numStreams, uploadSize, downloadSize, chunkSize, runs: int,
 ): Future[RunResult] {.async.} =
-  var result = RunResult(
+  var runResult = RunResult(
     mode: Stress,
     connections: numConns,
     streamsPerConn: numStreams,
@@ -293,24 +293,24 @@ proc modeStress(
       futConnIdx.add(ci)
 
     # Ensure we have connResults slots
-    while result.connResults.len < numConns:
-      result.connResults.add(ConnectionResult())
+    while runResult.connResults.len < numConns:
+      runResult.connResults.add(ConnectionResult())
 
     for i, f in allFuts:
       let sr = await f
-      result.connResults[futConnIdx[i]].streamResults.add(sr)
+      runResult.connResults[futConnIdx[i]].streamResults.add(sr)
 
   let totalDur = (Moment.now() - start).nanoseconds
-  for cr in result.connResults.mitems:
+  for cr in runResult.connResults.mitems:
     cr.durationNs = totalDur
-  result.durationNs = totalDur
+  runResult.durationNs = totalDur
 
   for conn in conns:
     conn.close()
   for client in clients:
     await client.stop()
 
-  return result
+  return runResult
 
 # -- Mode: rampup (1 conn, 1 stream, measure throughput over time) --
 
@@ -321,7 +321,7 @@ const
 proc modeRampUp(
     serverAddr: TransportAddress, downloadSize: int, chunkSize: int
 ): Future[RunResult] {.async.} =
-  var result = RunResult(
+  var runResult = RunResult(
     mode: RampUp,
     connections: 1,
     streamsPerConn: 1,
@@ -417,24 +417,25 @@ proc modeRampUp(
 
   var connRes =
     ConnectionResult(streamResults: @[sr], durationNs: totalDuration.nanoseconds)
-  result.connResults.add(connRes)
-  result.durationNs = totalDuration.nanoseconds
+  runResult.connResults.add(connRes)
+  runResult.durationNs = totalDuration.nanoseconds
 
   conn.close()
   await client.stop()
-  return result
+  return runResult
 
 # -- Print results --
 
-proc printResults(result: RunResult) =
+proc printResults(runResult: RunResult) =
   echo ""
   echo "=== Benchmark Results ==="
-  echo "Mode: ", result.mode
-  echo "Connections: ", result.connections, ", Streams/conn: ", result.streamsPerConn
-  echo "Total duration: ", formatDuration(result.durationNs)
+  echo "Mode: ", runResult.mode
+  echo "Connections: ",
+    runResult.connections, ", Streams/conn: ", runResult.streamsPerConn
+  echo "Total duration: ", formatDuration(runResult.durationNs)
   echo ""
 
-  for ci, cr in result.connResults:
+  for ci, cr in runResult.connResults:
     echo "  Connection #", ci + 1, ":"
 
     for si, sr in cr.streamResults:

benchmarks/bench_common.nim

@@ -175,7 +175,7 @@ proc toJson*(r: RunResult): JsonNode =
         streamNode["time_to_p90_ns"] = %sr.timeToP90Ns
       streamArr.add(streamNode)
     connArr.add(%*{"streams": streamArr, "duration_ns": cr.durationNs})
-  result = %*{
+  var json = %*{
     "mode": $r.mode,
     "connections": r.connections,
     "streams_per_conn": r.streamsPerConn,
@@ -185,3 +185,4 @@ proc toJson*(r: RunResult): JsonNode =
     "duration_ns": r.durationNs,
     "connections_results": connArr,
   }
+  json

lsquic/client.nim

@@ -6,8 +6,8 @@ import ./[errors, connection, tlsconfig, endpoint]
 
 type QuicClient* = ref object of RootObj
   tlsConfig: TLSConfig
-  endpoint4: QuicEndpoint
-  endpoint6: QuicEndpoint
+  ip4Endpoint: QuicEndpoint
+  ip6Endpoint: QuicEndpoint
 
 proc new*(t: typedesc[QuicClient], tlsConfig: TLSConfig): QuicClient {.raises: [].} =
   QuicClient(tlsConfig: tlsConfig)
@@ -17,15 +17,15 @@ proc getEndpoint(
 ): QuicEndpoint {.raises: [QuicError, TransportOsError].} =
   case family
   of AddressFamily.IPv4:
-    if self.endpoint4.isNil:
-      self.endpoint4 = QuicEndpoint.new(self.tlsConfig, family)
+    if self.ip4Endpoint.isNil:
+      self.ip4Endpoint = QuicEndpoint.new(self.tlsConfig, family)
 
-    return self.endpoint4
+    return self.ip4Endpoint
   of AddressFamily.IPv6:
-    if self.endpoint6.isNil:
-      self.endpoint6 = QuicEndpoint.new(self.tlsConfig, family)
+    if self.ip6Endpoint.isNil:
+      self.ip6Endpoint = QuicEndpoint.new(self.tlsConfig, family)
 
-    return self.endpoint6
+    return self.ip6Endpoint
   else:
     raise newException(QuicError, "client supports only IPv4/IPv6 address")
 
@@ -37,9 +37,11 @@ proc dial*(
   await self.getEndpoint(address.family).dial(address)
 
 proc stop*(self: QuicClient) {.async: (raises: [CancelledError]).} =
-  if not self.endpoint4.isNil:
-    await noCancel self.endpoint4.stop()
-  if not self.endpoint6.isNil:
-    await noCancel self.endpoint6.stop()
-  self.endpoint4 = nil
-  self.endpoint6 = nil
+  var stops: seq[Future[void]]
+  if not self.ip4Endpoint.isNil:
+    stops.add(noCancel self.ip4Endpoint.stop())
+  if not self.ip6Endpoint.isNil:
+    stops.add(noCancel self.ip6Endpoint.stop())
+  await allFutures(stops)
+  self.ip4Endpoint = nil
+  self.ip6Endpoint = nil

lsquic/connectionmanager.nim

@@ -23,7 +23,7 @@ proc stop*(connman: ConnectionManager) {.async: (raises: [CancelledError]).} =
   for conn in active:
     conn.abort()
 
-proc removeConnection(connman: ConnectionManager, conn: Connection) {.raises: [].} =
+proc removeConnection*(connman: ConnectionManager, conn: Connection) {.raises: [].} =
   for i in 0 ..< connman.connections.len:
     if connman.connections[i] == conn:
       let last = connman.connections.high