juno
juno copied to clipboard
jsonrpc: implement unix handler
closes #1277
Codecov Report
Attention: 40 lines
in your changes are missing coverage. Please review.
Comparison is base (
e9e9a57
) 73.05% compared to head (0f0778e
) 73.20%.
:exclamation: Current head 0f0778e differs from pull request most recent head b7ade80. Consider uploading reports for the commit b7ade80 to get more accurate results
Files | Patch % | Lines |
---|---|---|
jsonrpc/ipc.go | 81.81% | 16 Missing and 6 partials :warning: |
node/http.go | 74.46% | 9 Missing and 3 partials :warning: |
node/metrics.go | 75.00% | 2 Missing and 1 partial :warning: |
node/node.go | 57.14% | 2 Missing and 1 partial :warning: |
Additional details and impacted files
@@ Coverage Diff @@
## main #1362 +/- ##
==========================================
+ Coverage 73.05% 73.20% +0.14%
==========================================
Files 98 98
Lines 10101 10209 +108
==========================================
+ Hits 7379 7473 +94
- Misses 2174 2186 +12
- Partials 548 550 +2
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Hey, thanks for the contribution!
Do you have an immediate use case for this? Because I am not sure if the marginal speed up that IPC brings, on its own, warrants a new feature to be added over TCP.
I usually mount the socket instead of using loopback since it saves 2(docker+interface itself) hops for containerized env and is generally easier to manage.
I usually mount the socket instead of using loopback since it saves 2(docker+interface itself) hops for containerized env and is generally easier to manage.
Fair enough
I'll try to review this sometime next week. May take a bit longer depending on prioritization.
Made a first pass. Should Juno be able to communicate over multiple unix sockets, each representing a different IPC connection?
Spawning a socket for each connection is overkill I believe, one is enough. Spawning multiple sockets can be considered for keeping multiple jsonrpc.server
to align with current http and ws rpc (jsonrpcServerLegacy
and jsonrpcServer
) behavior. The problem here is that socket creation can fail in either of those and I didn't know what expected behavior would be in that case so I just spawned the latest one.
Spawning a socket for each connection is overkill I believe, one is enough.
Understood, thanks.
The problem here is that socket creation can fail in either of those and I didn't know what expected behavior would be in that case so I just spawned the latest one.
That's ok, creating TCP sockets can also fail. Let's maintain consistency with websocket and http handlers, and spawn a socket for each server.
Sorry for the two lint commits, got confused by that:
Locally make lint
returns directive //nolint:funlen is unused for linter "funlen" (nolintlint)
dev@dev:~/contrib/juno$ golangci-lint --version
golangci-lint has version v1.54.2 built with go1.21.0 from (unknown, mod sum: "h1:oR9zxfWYxt7hFqk6+fw6Enr+E7F0SN2nqHhJYyIb0yo=") on (unknown)
Yet the check fails without that:
[lint: cmd/juno/juno.go#L150](https://github.com/NethermindEth/juno/pull/1362/files#annotation_15384594147)
Function 'NewCmd' has too many statements (51 > 50) (funlen)
No worries. It's flaky sometimes. Thanks for fixing.