Skip to content

refactor: flush after calling write_all #3614

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

Merged
merged 1 commit into from
May 15, 2025

Conversation

jarhodes314
Copy link
Contributor

@jarhodes314 jarhodes314 commented May 13, 2025

Proposed changes

Improve the file handling by:

  • Making sure we flush any buffers once we have written to the file
  • Making sure we use a BufWriter to improve file performance
  • For cases where we write all the contents at once (often the case in tests, use fs::write(...) instead of manually opening a file

There are probably many cases here where it's not strictly necessary to do a flush (anywhere where we close a file before we rely on the data being persisted), but there are definitely quite a few cases where we this assumption doesn't hold for us.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
  • Documentation Update (if none of the other choices apply)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue


Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA (in all commits with git commit -s. You can activate automatic signing by running just prepare-dev once)
  • I ran just format as mentioned in CODING_GUIDELINES
  • I used just check as mentioned in CODING_GUIDELINES
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

Copy link

codecov bot commented May 13, 2025

Codecov Report

Attention: Patch coverage is 61.90476% with 24 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
crates/common/tedge_utils/src/file.rs 27.27% 3 Missing and 5 partials ⚠️
crates/common/tedge_utils/src/paths.rs 0.00% 7 Missing ⚠️
plugins/c8y_remote_access_plugin/src/lib.rs 0.00% 6 Missing ⚠️
crates/core/plugin_sm/src/plugin.rs 0.00% 1 Missing ⚠️
crates/core/tedge/src/cli/certificate/create.rs 0.00% 0 Missing and 1 partial ⚠️
...ensions/tedge_log_manager/src/manager/log_utils.rs 75.00% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

github-actions bot commented May 13, 2025

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
633 0 3 633 100 1h48m9.756495s

@reubenmiller reubenmiller added the refactoring Developer value label May 13, 2025
@Bravo555 Bravo555 self-requested a review May 14, 2025 09:03
Copy link
Contributor

@Bravo555 Bravo555 left a comment

Choose a reason for hiding this comment

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

Looks alright, some flushes could maybe be syncs, but I'm not sure if it's necessary either way, so approving.

@@ -415,6 +415,7 @@ async fn save_chunks_to_file_at(
while let Some(bytes) = response.chunk().await? {
writer.write_all(&bytes)?;
}
writer.flush()?;
Copy link
Contributor

Choose a reason for hiding this comment

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

note: this is an example where there's no functional change, because flush on a File is a noop, but still it's probably good to have for consistency or if somebody added a buffer and forgot to add a flush call.

If we actually want to ensure that all writes reach the file, I think sync_data or sync_all might be better here.

@@ -137,6 +137,7 @@ impl CommonMosquittoConfig {

self.internal_listener.write(writer).await?;
self.external_listener.write(writer).await?;
writer.flush().await?;
Copy link
Contributor

Choose a reason for hiding this comment

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

note: if the caller calls serialize multiple times, then adding flush here means some of the flushes will be redundant. The point is to flush the writer before it's dropped, so the caller should probably do it either way, but keeping it here is also fine.

Copy link
Contributor

@didier-wenzek didier-wenzek left a comment

Choose a reason for hiding this comment

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

Approved

@jarhodes314 jarhodes314 force-pushed the refactor/flush-file-buffers branch from 3a96e3c to e108a93 Compare May 15, 2025 11:35
@jarhodes314 jarhodes314 temporarily deployed to Test Pull Request May 15, 2025 11:35 — with GitHub Actions Inactive
@jarhodes314 jarhodes314 enabled auto-merge May 15, 2025 11:36
@jarhodes314 jarhodes314 added this pull request to the merge queue May 15, 2025
Merged via the queue into thin-edge:main with commit a881370 May 15, 2025
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Developer value
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants