Skip to content
New issue

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

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

Already on GitHub? # to your account

Support custom ipc paths #8

Merged
merged 2 commits into from
Apr 13, 2021

Conversation

njgheorghita
Copy link
Collaborator

@njgheorghita njgheorghita commented Mar 29, 2021

Ready for review after #2 is merged

@carver carver force-pushed the custom-ipc-path branch from b0aa34f to 927b3c1 Compare April 6, 2021 00:05
src/cli.rs Outdated
_ => panic!("Must not supply an ipc path when using http protocol"),
},
"ipc" => match &http_port.to_string()[..] {
DEFAULT_HTTP_PORT => println!("Protocol: {}", protocol),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be nice to print the IPC path here.

src/jsonrpc.rs Outdated
Comment on lines 25 to 29
// TODO something smarter than just dropping the existing file and/or
// make sure file gets cleaned up on shutdown.
match fs::remove_file(ipc_path) {
Err(_) => panic!("Could not serve IPC from existing path '{}'", ipc_path),
Ok(()) => unix::net::UnixListener::bind(ipc_path).unwrap(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

So this approach no longer seems acceptable with a configurable IPC path (just dropping whatever file is found there and replacing it).

The simple answer is that we should just panic instead of trying to remove the file and reattempting. But then to solve the original problem: I think that we were leaking the file on shutdown sometimes. (Maybe anytime there's a panic?)

So the next step is to make sure that the IPC file gets removed when trin shuts down, even after a panic. (Although, it's probably worth verifying the theory. All I really know is that sometimes it used to happen that I had a pre-existing file on startup, and it was annoying. Not sure where it came from).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I updated the code to add some handlers that will clean up the ipc file upon ctrl+c shutdown and all panics. It works as expected locally, but could use some integration tests to make sure the cleanup happens as expected. I'm leaning towards saving those for a larger integration test pr coming soontm.

@carver carver marked this pull request as ready for review April 6, 2021 00:27
@carver
Copy link
Collaborator

carver commented Apr 6, 2021

I took the liberty of rebasing

@carver
Copy link
Collaborator

carver commented Apr 8, 2021

Reminder to rebase on master to get CI tests

@njgheorghita njgheorghita changed the title WIP - Support custom ipc paths Support custom ipc paths Apr 8, 2021
@njgheorghita njgheorghita force-pushed the custom-ipc-path branch 4 times, most recently from 17d807c to 91975de Compare April 8, 2021 10:54
@njgheorghita
Copy link
Collaborator Author

njgheorghita commented Apr 8, 2021

@carver ping for another 👀. I updated the settings in circle ci to "Build forked pull requests" and "Pass secrets to builds from forked pull requests" - both of which are necessary to get ci working here. FYI re: passing secrets -> "passing secrets to builds from forked pull requests can cause security vulnerabilities"- which seems fine since we're using a dummy infura key for now. Even when we use a real infura key for integration testing, imo it doesn't warrant a more secure setup, but it's something to keep in mind.

@njgheorghita njgheorghita requested a review from carver April 8, 2021 12:58
@njgheorghita njgheorghita merged commit 868b953 into ethereum:master Apr 13, 2021
@njgheorghita njgheorghita deleted the custom-ipc-path branch April 13, 2021 14:51
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extract client logic from cli module into new jsonrpc module Allow specifying the IPC location
2 participants