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

guard against throwing in random_access_file's write_datastream dtor #439

Merged
merged 1 commit into from
Aug 8, 2024

Conversation

spoonincode
Copy link
Member

I encountered an unfortunate design deficiency in random_access_file's write_datastream. Upon destruction write_datastream will flush its buffer via do_write() but that call can throw. When this occurs obviously it's Game Over.

This tweaks the interface to write_datastream: exceptions during dtor are swallowed (with a log warning); if a user of the class wants errors to be reported they must call a new flush() which still throws if writing the buffer fails.

I don't really like that and I'm open to other potential tweaks, but this pattern does have some precedent in ofstream (with basic_filebuf).

@@ -419,12 +433,14 @@ class random_access_file {
void pack_to(const T& v, const ssize_t offset) {
write_datastream ds(ctx, offset);
fc::raw::pack(ds, v);
ds.flush();
Copy link
Contributor

Choose a reason for hiding this comment

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

Could the ctx->write_to() on each pack_to() call hurt performance?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not in this case: the buffer is/was flushed upon destruction of write_datastream so the buffer was already being flushed at the end of pack_to().

@ericpassmore
Copy link
Contributor

Note:start
group: STABILITY
category: INTERNALS
summary: Guard against throwing an error write_deatastream's destructor.
Note:end

@spoonincode
Copy link
Member Author

An alternative might be to actually allow write_datastream's dtor to throw. I hadn't really considered that an option previously since that's considered a highly unusual (and discouraged) pattern, but it might be workable in this situation.

@spoonincode spoonincode merged commit c95db46 into main Aug 8, 2024
36 checks passed
@spoonincode spoonincode deleted the raf_nothrow_dtor branch August 8, 2024 17:29
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.

4 participants