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

Stop collecting disposables if changed token encountered #66265

Conversation

mapogolions
Copy link
Contributor

No description provided.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Mar 6, 2022
@ghost
Copy link

ghost commented Mar 6, 2022

Tagging subscribers to this area: @dotnet/area-extensions-primitives
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author: mapogolions
Assignees: -
Labels:

area-Extensions-Primitives, community-contribution

Milestone: -

@eerhardt
Copy link
Member

eerhardt commented Mar 7, 2022

Thank you for the PR, @mapogolions. Can you add a description to this PR explaining what the situation is that is not behaving correctly today, and why it isn't behaving correctly?

@mapogolions
Copy link
Contributor Author

@eerhardt

Traditional flow:

We usually pass a list of tokens that have not yet been changed to the constructor of the composite token.

var compositeToken = new CompositeChangeToken(token1, token2, ..., tokenN);

where: token1.HasChanged is False, token2.HasChanged is False, ..., tokenN.HasChanged is False

Then we register some callback for the composite token.

compositeToken.Register(...);

at this moment compositeToken.HasChanged is False

At the output, we have a construct in which the created composite token has not yet been changed
and each of the passed tokens (token1, token2, ..., tokenN) can initiate a change in the composite token in the future.

If cancellation is requested for one of these tokens

tokenSource1.Cancel()

the static OnChange method will be called. What it does:

  1. lock and cancel composite token
  2. Dispose all disposables (registrations)

If cancellation is then requested for the second passed token

tokenSource2.Cancel()

The OnChange method will no longer be called as all disposables(registrations) have been disposed.

Issue flow:

We also pass a list of tokens, but one of them has already been changed.

var compositeToken = new CompositeChnageToken(token1, token2, ..., tokenN);

where: token1.HasChanged is True, token2.HasChanged is False, ..., tokenN.HasChanged is False

Then we register some callback for the composite token

compositeToken.Register(...)

The static method OnChange will be called at the moment of registration by the line above.
As result, we have a changed composite token (e.g. compositeToken.HasChanged is True), but we also have a list of disposables
that hasn't been disposed.
And If cancellation is then requested for the second passed token

tokenSource2.Cancel()

It will trigger the OnChange method again.

This PIR is trying to remove redundant work - if we know that the composite token has changed, why do we still keep registering a callback for each token in order to be able to change the composite token in the future

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

LGTM. Just a couple minor code style changes.

FYI @davidfowl - in case you have any concerns here.

@mapogolions
Copy link
Contributor Author

@eerhardt Thanks for review. I am still working on my English. I would appreciate any of your suggestions for renaming local variables, classes, functions.

@eerhardt
Copy link
Member

I would appreciate any of your suggestions for renaming local variables, classes, functions.

I thought they all looked fine.

@eerhardt
Copy link
Member

Test failure was #65791.

@eerhardt eerhardt merged commit 67ea9c0 into dotnet:main Mar 10, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Apr 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Extensions-Primitives community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants