-
Notifications
You must be signed in to change notification settings - Fork 13
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
Delete log file #2442
base: develop
Are you sure you want to change the base?
Delete log file #2442
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 10 files at r1.
Reviewable status: 1 of 10 files reviewed, all discussions resolved (waiting on @doums, @octol, @rokas-ambrazevicius, and @zaneschepke)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 10 files reviewed, 1 unresolved discussion (waiting on @doums, @octol, @rokas-ambrazevicius, and @zaneschepke)
nym-vpn-core/crates/nym-vpnd/src/logging.rs
line 88 at r1 (raw file):
let mut file_path = service::log_dir(); file_path.push(service::DEFAULT_LOG_FILE); let Ok(mut file_lock) = self.logging_setup.file_appender.lock() else {
Use tokio mutex in async contexts, it supports both blocking and non-blocking locking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 10 files reviewed, 2 unresolved discussions (waiting on @doums, @neacsu, @octol, @rokas-ambrazevicius, and @zaneschepke)
nym-vpn-core/crates/nym-vpnd/src/logging.rs
line 71 at r1 (raw file):
Some(_) = self.tunnel_event_rx.recv() => { tracing::debug!("Received command to delete log file"); self.handle_delete_log_file();
This calls into function using std Mutex
. Prefer async mutex from tokio.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 10 files reviewed, 3 unresolved discussions (waiting on @doums, @neacsu, @octol, @rokas-ambrazevicius, and @zaneschepke)
nym-vpn-core/crates/nym-vpnd/src/logging.rs
line 96 at r1 (raw file):
drop(file_ref); } if let Err(err) = std::fs::remove_file(file_path) {
Prefer async friendly tokio::fs to avoid blocking runtime
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 10 files reviewed, 4 unresolved discussions (waiting on @doums, @neacsu, @octol, @rokas-ambrazevicius, and @zaneschepke)
nym-vpn-core/crates/nym-vpnd/src/logging.rs
line 92 at r1 (raw file):
return; }; if let Some(file_ref) = file_lock.take() {
Nit: you can do _ = file.lock.take();
which would identical but shorter. But this is a matter of preference of course
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 10 files reviewed, 3 unresolved discussions (waiting on @doums, @octol, @pronebird, @rokas-ambrazevicius, and @zaneschepke)
nym-vpn-core/crates/nym-vpnd/src/logging.rs
line 71 at r1 (raw file):
Previously, pronebird (Andrej Mihajlov) wrote…
This calls into function using std
Mutex
. Prefer async mutex from tokio.
Done.
nym-vpn-core/crates/nym-vpnd/src/logging.rs
line 88 at r1 (raw file):
Previously, pronebird (Andrej Mihajlov) wrote…
Use tokio mutex in async contexts, it supports both blocking and non-blocking locking.
Good point
nym-vpn-core/crates/nym-vpnd/src/logging.rs
line 96 at r1 (raw file):
Previously, pronebird (Andrej Mihajlov) wrote…
Prefer async friendly tokio::fs to avoid blocking runtime
Done.
Tested on macOS, works 👍 |
Empty log file for desktop platforms, when writing to the filesystem (running as a service)
This change is