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

Async dispose pattern mini fixes #26269

Closed

Conversation

deep-outcome
Copy link
Contributor

@deep-outcome deep-outcome commented Sep 26, 2021

Summary

In commit order:

  1. Unacceptable pattern – managed resource must be handled only in managed resources disposal block.
  2. Same.
  3. More clear definitions on DisposeAsync and DisposeAsyncCore. Code showing nothing unsaid removal.
  4. Clarification on place of disposal, it is likely not known if variables refer to any instances (they were [even] checked for).
  5. What happens on producer side (AnotherAsyncDisposable) is not conditional from-by consumer pattern. Thus is irrelevant to denote any responsibilities resulting from bad pattern on consumer side. It is not important whether variable is assigned or not. Important is that valid instance referred to by objOne DisposeAsync is not called while it should and can be.

Check also #26270.

@IEvangelist IEvangelist self-assigned this Sep 29, 2021
Copy link
Member

@IEvangelist IEvangelist left a comment

Choose a reason for hiding this comment

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

This looks good, thank you. We'll :shipit: when you make the suggested changes.


:::code language="csharp" id="dontdothis" source="snippets/dispose-async/ExamplePatterns.cs" highlight="9-10":::

> [!TIP]
> Avoid this pattern as it could lead to unexpected behavior.
Copy link
Member

Choose a reason for hiding this comment

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

Why is this being removed? This should stay.

Copy link
Contributor Author

@deep-outcome deep-outcome Sep 29, 2021

Choose a reason for hiding this comment

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

Please, this is eligible for removal since it gives a tip on phenomenon that is focused in whole chapter. I my casual day I would not appreciate a tip on anything being right 100 % obvious.

Saving that regular C# programmer would not be recognizing an unacceptable pattern as not avoidance-worthy.

Copy link
Member

Choose a reason for hiding this comment

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

➡️ Users frequently search for code, and then copy paste whichever result shows up without reading all the surrounding text. The callout here is intended to highlight (via color/page style) the fact that copying this result is problematic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm… If I skip that right above is quite big heading Unacceptable pattern should not this be a note then?

@IEvangelist
Copy link
Member

Hi again @Uzivatel919 - in discussing the proposed changes with both @bartonjs and @sharwell, they confirmed that the moving for the null assignment is not necessary. For consistency with the tooling / analyzer comments that are provided during the implementation refactoring of both Dispose and DisposeAsync I'm going to close this PR. Thanks again for the contribution and having an open dialog.

@deep-outcome
Copy link
Contributor Author

Hi again @Uzivatel919 - in discussing the proposed changes with both @bartonjs and @sharwell, they confirmed that the moving for the null assignment is not necessary. For consistency with the tooling / analyzer comments that are provided during the implementation refactoring of both Dispose and DisposeAsync I'm going to close this PR. Thanks again for the contribution and having an open dialog.

@IEvangelist, @bartonjs, @sharwell, I have to take an objection. Moving null assignment into respective manage resources disposal block is not really necessary by virtue of code flow. On the other hand I would say anyone checking with these docs would be expecting that examples of pattern follows the patterns. In this case, separation of logic manipulating un/managed resource to respective code blocks.

@IEvangelist, here are also another changes beside moving null assignments. Their externality have not been explained.

@deep-outcome deep-outcome deleted the AsyncDisposePatternMiniFixes branch September 30, 2024 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants