autofac / Documentation

Usage and API documentation for Autofac and integration libraries
http://autofac.readthedocs.org
MIT License
71 stars 121 forks source link

Is the important part of the IAsyncDisposable section in the disposal.rst still true? #154

Closed romerod closed 2 years ago

romerod commented 2 years ago

The following part in https://github.com/autofac/Documentation/blob/master/docs/lifetime/disposal.rst seems to be wrong:

If your component only implements IAsyncDisposable, but someone disposes of the scope synchronously, then Autofac will throw an exception, because it does not know how to dispose of your component.

Warnings are shown but no exceptions are thrown:

AUTOFAC: A synchronous Dispose has been attempted, but the tracked object of type 'XXXX' only implements IAsyncDisposable. This will result in an efficient blocking dispose. Consider either implementing IDisposable on 'UserQuery+TestDisposable' or disposing of the scope/container with DisposeAsync. DisposedAsync

Related code: https://github.com/autofac/Autofac/blob/be2a437ca048a7534a5ed7936334d1336291aa1d/src/Autofac/Core/Disposer.cs#L51

https://github.com/autofac/Autofac/commit/3b6f6848396c2363dfcc07ece0b28c86a0ec1ba6#diff-24e2ed8f0c27b3ee3221361170a492f1ec2161d73c1dff50596282b064428d69

tillig commented 2 years ago

Oh, yup, we changed that because we ran into some components that only implement IAsyncDisposable (e.g., ServiceBusClient) and it caused issues. Looks like the corresponding docs need to be updated. Which is to say, if the component is under the dev's control, yeah, implement both IDisposable and IAsyncDisposable so it does the right thing, but there were some things outside our control and not part of the developer's code, so throwing seemed like it could be problematic.

romerod commented 2 years ago

I don't actually see, how implementing both would change anything, unless the some work can be done with a synchronous API of course. Wouldn't at the end the result also be an inefficient blocking dispose? Just that the code moved to the user code. Supposing that in most of the cases where IAsyncDisposable is implemented it was chosen to do some asynchronous work.

From my point of view the warning/important info should be something like use DisposeAsync on the scope if possible.

tillig commented 2 years ago

I think the solution here is simply to remove the warning from the docs rather than reword it or provide different guidance. Everything should just work now.

alistairjevans commented 2 years ago

Agreed; we can probably just remove that content about throwing an exception, there's already content that says what happens when calling DisposeAsync vs Dispose.