juno icon indicating copy to clipboard operation
juno copied to clipboard

jsonrpc: implement unix handler

Open Exca-DK opened this issue 1 year ago • 9 comments

closes #1277

Exca-DK avatar Oct 24 '23 05:10 Exca-DK

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.

codecov[bot] avatar Oct 24 '23 05:10 codecov[bot]

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.

omerfirmak avatar Oct 25 '23 10:10 omerfirmak

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.

Exca-DK avatar Oct 26 '23 04:10 Exca-DK

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

omerfirmak avatar Oct 26 '23 10:10 omerfirmak

I'll try to review this sometime next week. May take a bit longer depending on prioritization.

joshklop avatar Oct 28 '23 21:10 joshklop

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.

Exca-DK avatar Oct 30 '23 22:10 Exca-DK

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.

joshklop avatar Oct 31 '23 22:10 joshklop

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)

Exca-DK avatar Nov 02 '23 21:11 Exca-DK

No worries. It's flaky sometimes. Thanks for fixing.

joshklop avatar Nov 03 '23 00:11 joshklop