Conversation
tautropfli
force-pushed
the
update-tooling
branch
from
08149b4 to
011800c
Compare
|
@Mafii It's probably easiest to review the individual commits ^^ |
| public static TheoryData<Disposable<DisposableStub>> Disposables() | ||
| => [ | ||
| Disposable.Owned(new DisposableStub()), | ||
| Disposable.Borrowed(new DisposableStub()), |
There was a problem hiding this comment.
I don't see why this test case should throw on double dispose, isn't this the point, that it is not disposed when borrowed? Maybe I just don't understand.
There was a problem hiding this comment.
Yes, the inner disposable is not disposed when borrowed. What I changed is that the wrapper tracks that it got disposed and throws in that case. I thought it would be a good thing for consistency since that's what other disposables do too.
There was a problem hiding this comment.
Oh, never mind I misunderstood the dispose pattern 😆
Dispose should do nothing when called multiple times, but anything that accesses the resource should throw.
There was a problem hiding this comment.
I'm not sure we should implement this for our consumers. I know this is correct according the dispose pattern as per Microsoft, but if we track dispose, the contained value can't decide if it throws when used after dispose.
This is a huge change and not necessarily in the interest of consumers, I fear? What do you think? There might be existing code that doesn't care if dispose is called, and then take after. This would be a breaking change for them, at least theoretically.
There was a problem hiding this comment.
I agree it might be a bit of a change, but I think it's a good one :)
Consumers of the disposable wrapper currently see two different behaviours when the wrapper is disposed:
- if owned, they might get an
ObjectDisposedExceptionwhen trying to do anything on the inner object - if borrowed, they don't
One of the use cases of the disposable is to treat owned and borrowed objects the same way. Always throwing if disposed might help catch bugs :)
Dispownership/AsyncDisposable.cs
Outdated
| return ValueTask.FromException(new ObjectDisposedException(nameof(AsyncDisposable))); | ||
| } | ||
|
|
||
| _disposed = true; |
There was a problem hiding this comment.
This does exactly what the test shows - but I'm not sure it is correct, sorry!
global.json
Outdated
| "rollForward": "feature" | ||
| } | ||
| "sdk": { | ||
| "version": "9.0.100", |
There was a problem hiding this comment.
You might have to bump this to .102 for the pipeline to work, 101 and 100 are not usable through actions/ci anymore (marked as insecure by msft)
|
I currently don't have the bandwidth to continue on this PR, closing :) |
2 participants
No description provided.