Change close_rc()'s API

[waynexia]

What does this PR do?

Change close_rc to return Result<bool> to tell whether an underlying file is actually closed. And no error if isn't.

Motivation

For situations that wrap DmaFile with Rc, we usually have many tasks that will access the file. Sometimes it's hard to tell which one will be the last finished and is responded to close the file. A natural way IMO would be for each task (which keeps an Rc<DmaFile> handle) to try to close their Rc handle via close_rc, and only the last will actually close the file.

In the current API however, we need to check the strong counter one more time out of close_rc() or match the returning error that can a caller tell if the error is from closing a file or just there is another copy of the same Rc.

Related issues

A list of issues either fixed, containing architectural discussions, otherwise relevant
for this Pull Request.

Additional Notes

Anything else we should know when reviewing?

Checklist

[] I have added unit tests to the code I am submitting
[] My unit tests cover both failure and success scenarios
[] If applicable, I have discussed my architecture

[glommer]

I don't understand what is the benefit of this.
Err() should already encapsulate that the file was not closed.

Is your concern distinguishing between errors arising due to the strong count being high versus actual errors closing the file ?

[waynexia]

Thanks for responding @glommer :D

To be honest there is no benefit for functionality and correctness. But IMO this makes the API a little bit easier to use.

Is your concern distinguishing between errors arising due to the strong count being high versus actual errors closing the file ?

Yes, take the changed file for example, what it wants to ignore is the "not really closed" error, but the underlying error is also ignored. And I think it is more common to "just drop the Rc, and close the file if it's the last one". So I made this change to not treat "not really closed" as an error but a boolean flag as a return value.

[duarten]

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.

[HippoBaro]

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> { ... }

[duarten]

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.

[waynexia]

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

[HippoBaro]

I am sorry I don't understand the semantics of the CloseResult wrapped in a Result. It's an enum with a variant signaling success and yet it is only used on the error code path. Additionally, your solution doesn't forward the DmaFile::close() error to the user, if any. That's a missed opportunity.

I still think this is a better solution:

#[derive(Debug)]
enum CloseFailedError {
    // Unwrapping the Rc failed
    FileIsShared,
    // Closing the file failed so we return the actual GlommioError with the
    // details of the failure
    CloseFailed(GlommioError),
}
pub async fn close_rc(self: Rc<DmaFile>) -> Result<CloseFailedError> { ... }

In the above, we get the best of both worlds. You return Ok if everything went well, or Err(CloseFailedError::...) if something went wrong. The user can then look at the error to determine and differentiate between soft errors (file shared) and hard ones (closing failed for various reasons). In the latter case, they can extract the GlommioError and do proper error handling.

[duarten]

I don't understand why we should treat the unreferenced case as an error? If forces code like:

match f.close_rc() {
    Err(CloseFailedError::CloseFailed(e)) => return Err(e),
    _ => { }
}

Instead of the more natural:

f.close_rc()?

Any DmaFile::close() error is propagated by file.close().await.map(|_| CloseResult::Closed).

[HippoBaro]

Because it can be useful for the user to know whether or not the operation was carried out. I can imagine use cases where the user want to assert that the file was indeed closed and no longer used elsewhere. This approach gives more information to the user.

[duarten]

Because it can be useful for the user to know whether or not the operation was carried out. I can imagine use cases where the user want to assert that the file was indeed closed and no longer used elsewhere. This approach gives more information to the user.

It gives the same information as what's currently implemented in the PR:

match f.close_rc() {
    Ok(CloseResult::Closed) => { ... } // file actually closed
    Ok(CloseResult::Unreferenced) => { ... } // ref counter decremented
    Err(e) => { ... } // an actual error
}

The only difference is it models the unreferenced case as an error, which I still don't understand why is desirable, given that it is not semantically one, and makes the common case of just applying ? harder.

[HippoBaro]

🤦 I misread the code. The PR does exactly what it needs to. I can see how your approach makes consuming the result simpler. Sorry!