Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

jsonrpc: implement unix handler #1362

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Exca-DK
Copy link
Contributor

@Exca-DK Exca-DK commented Oct 24, 2023

closes #1277

@codecov
Copy link

codecov bot commented Oct 24, 2023

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%.

❗ 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 ⚠️
node/http.go 74.46% 9 Missing and 3 partials ⚠️
node/metrics.go 75.00% 2 Missing and 1 partial ⚠️
node/node.go 57.14% 2 Missing and 1 partial ⚠️
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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@omerfirmak
Copy link
Contributor

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.

@Exca-DK
Copy link
Contributor Author

Exca-DK commented Oct 26, 2023

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.

@omerfirmak
Copy link
Contributor

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

@joshklop joshklop self-requested a review October 28, 2023 21:23
@joshklop
Copy link
Contributor

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

Copy link
Contributor

@joshklop joshklop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made a first pass. Should Juno be able to communicate over multiple unix sockets, each representing a different IPC connection?

jsonrpc/ipc.go Outdated Show resolved Hide resolved
jsonrpc/ipc_all.go Outdated Show resolved Hide resolved
jsonrpc/ipc_all.go Outdated Show resolved Hide resolved
jsonrpc/ipc_all.go Outdated Show resolved Hide resolved
@Exca-DK
Copy link
Contributor Author

Exca-DK commented Oct 30, 2023

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.

@joshklop
Copy link
Contributor

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.

jsonrpc/ipc.go Outdated Show resolved Hide resolved
jsonrpc/ipc.go Outdated Show resolved Hide resolved
jsonrpc/ipc.go Outdated Show resolved Hide resolved
jsonrpc/ipc.go Outdated Show resolved Hide resolved
@Exca-DK
Copy link
Contributor Author

Exca-DK commented Nov 2, 2023

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)

@joshklop
Copy link
Contributor

joshklop commented Nov 3, 2023

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

jsonrpc/ipc_all.go Outdated Show resolved Hide resolved
jsonrpc/ipc.go Outdated Show resolved Hide resolved
jsonrpc/ipc.go Outdated Show resolved Hide resolved
jsonrpc/ipc.go Outdated Show resolved Hide resolved
jsonrpc/ipc.go Outdated Show resolved Hide resolved
jsonrpc/ipc.go Outdated Show resolved Hide resolved
jsonrpc/ipc_test.go Outdated Show resolved Hide resolved
node/http.go Outdated Show resolved Hide resolved
node/http.go Outdated Show resolved Hide resolved
node/http.go Show resolved Hide resolved
@Exca-DK Exca-DK requested a review from joshklop December 7, 2023 21:53
jsonrpc/ipc.go Outdated Show resolved Hide resolved
node/http.go Outdated Show resolved Hide resolved
@Exca-DK Exca-DK requested a review from joshklop December 14, 2023 21:46
Copy link
Contributor

@joshklop joshklop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Works almost perfectly on my machine. Just a few minor things left.

Mind if a team member rebases and/or makes small adjustments when we're ready to merge? I'm not sure if we'll fit it into a release right away.

jsonrpc/ipc_all.go Outdated Show resolved Hide resolved
jsonrpc/ipc_all.go Outdated Show resolved Hide resolved
jsonrpc/ipc_all.go Outdated Show resolved Hide resolved
jsonrpc/ipc_all.go Outdated Show resolved Hide resolved
jsonrpc/ipc.go Outdated Show resolved Hide resolved
node/http.go Outdated Show resolved Hide resolved
jsonrpc/ipc_test.go Outdated Show resolved Hide resolved
jsonrpc/ipc_test.go Show resolved Hide resolved
jsonrpc/ipc_test.go Outdated Show resolved Hide resolved
jsonrpc/ipc.go Outdated Show resolved Hide resolved
jsonrpc/ipc.go Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

Unix socket support
4 participants