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

Implement --ephemeral CLI flag #277

Closed
wants to merge 2 commits into from
Closed

Conversation

hmrtn
Copy link
Contributor

@hmrtn hmrtn commented Mar 26, 2022

What was wrong?

This Fixes #81, implementing the --ephemeral CLI flag.

How was it fixed?

This was fixed by creating a mutable singleton vector that stores the session's data path on startup and utilizes it in utils::db::cleanup_data_dir, which deletes the ephemeral directories on signal interruption (if the --ephemeral CLI flag is enabled).

Trin's signal interruption is already being handled in the service.rs via setup_ipc_cleanup_handlers(), and can only be called once. In order to minimize the amount of changes, a call to cleanup_data_dir() has been added here.

To-Do

  • Add entry to the release notes (may forgo for trivial changes)
  • Review thread safety and lock() usage with mutable global variables

@hmrtn hmrtn changed the title [WIP] Implement --ephemeral CLI flag Implement --ephemeral CLI flag Mar 26, 2022
@hmrtn hmrtn force-pushed the ephemeral-flag branch 2 times, most recently from 6a0f961 to 5b4e10a Compare April 21, 2022 03:38
@hmrtn hmrtn mentioned this pull request Apr 21, 2022
Copy link
Collaborator

@njgheorghita njgheorghita left a comment

Choose a reason for hiding this comment

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

Looks good!

trin-core/src/utils/db.rs Show resolved Hide resolved
rustfmt

rm target build

added: newsfragment

renamed trin data dirs and cargo fmt

spelling

comment: todo message

cargo fmt
Copy link
Member

@ogenev ogenev left a comment

Choose a reason for hiding this comment

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

Setting the cleanup handler inside json-rpc IPC exiter will not trigger if we use HTTP json-rpc transport.
For me, it makes more sense to create a general tokio exit handler function and set the cleanup handler right after we receive ctrl-c signal in tokio as described in this example: https://docs.rs/tokio/latest/tokio/signal/fn.ctrl_c.html#examples.

async fn handle_tokio_exit() {
    tokio::signal::ctrl_c()
            .await
            .expect("failed to pause until ctrl-c");

    cleanup_data_dir();
}

Then we can use this function in src/main.rs, trin-history/src/main.rs and trin-state/src/main.rs to wait for ctr-c and cleanup the directories.

@@ -86,6 +87,7 @@ fn set_ipc_cleanup_handlers(ipc_path: &str) {
if let Err(err) = fs::remove_file(&ipc_path) {
debug!("Ctrl-C: Skipped removing {} because: {}", ipc_path, err);
};
cleanup_data_dir();
Copy link
Member

Choose a reason for hiding this comment

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

This will not trigger if we initialize the node with HTTP json-rpc transport instead of IPC.

This was referenced Jun 28, 2022
@ogenev
Copy link
Member

ogenev commented Jun 30, 2022

Going to close this PR as it is superseded by #364 because we are quite in a rush to implement this feature.

Thank you @hmrtn for the initial work!

@ogenev ogenev closed this Jun 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement --ephemeral flag
3 participants