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.

Sign up for GitHub

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

Jump to bottom

Change close_rc()'s API #414

Merged
merged 5 commits into from
Sep 28, 2021
Merged

Change close_rc()'s API #414

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
30c9d4a
change close_rc() to return Result<bool>
waynexia Sep 22, 2021
41a5f4c
update doc
waynexia Sep 22, 2021
775d8e8
add enum CloseResult
waynexia Sep 26, 2021
629b42a
Merge branch 'master' into close-rc
HippoBaro Sep 27, 2021
dead85e
Merge branch 'master' into close-rc
HippoBaro Sep 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 6 additions & 8 deletions glommio/src/io/dma_file.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,15 +406,13 @@ impl DmaFile {
self.file.path()
}

/// Convenience method that closes a DmaFile wrapped inside an Rc
pub async fn close_rc(self: Rc<DmaFile>) -> Result<()> {
/// Convenience method that closes a DmaFile wrapped inside an Rc.
///
/// Returns whether the file is actually closed, or just dropped an Rc.
pub async fn close_rc(self: Rc<DmaFile>) -> Result<bool> {
Copy link
Member

Choose a reason for hiding this comment

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

It would be clearer to introduce an enum, something like

#[derive(Debug)]
enum CloseResult {
    Closed,
    Unreferenced,
    Error(Err),
}

This could be reused in ImmutableFile::close.

Copy link
Member

Choose a reason for hiding this comment

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

This looks weird to me. If we want to give the user that information we should do so through a Result so that we can propagate normally using ?. i.e.

#[derive(Debug)]
enum CloseFailedError {
    Unreferenced,
    CloseFailed(GlommioError),
}
pub async fn close_rc(self: Rc<DmaFile>) -> Result<CloseResult> { ... }

Copy link
Member

@duarten duarten Sep 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

One of those cases isn't an error, so shouldn't be treated as such. Not being able to use ? is an inconvenience, but eventually we'll be able to implement Try.

Moving the Unreferenced variant to CloseResult sounds good though:

#[derive(Debug)]
enum CloseResult {
    Closed,
    Unreferenced,
}

It's not like people will care about the distinction that often I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point. An enum is more clear than a boolean.

match Rc::try_unwrap(self) {
Err(file) => Err(io::Error::new(
io::ErrorKind::Other,
format!("{} references to file still held", Rc::strong_count(&file)),
)
.into()),
Ok(file) => file.close().await,
Err(_) => Ok(false),
Ok(file) => file.close().await.map(|_| true),
}
}
}
Expand Down
5 changes: 1 addition & 4 deletions glommio/src/io/dma_file_stream.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -410,10 +410,7 @@ impl DmaStreamReader {
let to_cancel = handles.len();
let cancelled = stream::iter(handles).then(|f| f).count().await;
assert_eq!(to_cancel, cancelled);

// the file may be used by other processes so failing to close it is not an
// issue.
let _ = self.file.close_rc().await;
self.file.close_rc().await.unwrap();

let mut state = self.state.borrow_mut();
match state.error.take() {
Expand Down
5 changes: 2 additions & 3 deletions glommio/src/io/immutable_file.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,10 +428,9 @@ impl ImmutableFile {
}

/// Closes this [`ImmutableFile`]
/// Note that this method returns an error if other entities hold references
/// to the underlying file, such as read streams.
pub async fn close(self) -> Result<()> {
self.stream_builder.file.close_rc().await
self.stream_builder.file.close_rc().await?;
Ok(())
}

/// Creates a [`DmaStreamReaderBuilder`] from this `ImmutableFile`.
Expand Down