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

change to ResetValidationTokenSource to guard against a disposed validationTokenSource #2344

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

matt-bartholomew
Copy link

@matt-bartholomew matt-bartholomew commented Nov 14, 2024

Description of Change

Reset validation token source may be called on a disposed validationTokenSource, resulting in an objectdisposedexception.

Linked Issues

PR Checklist

  • Has a linked Issue, and the Issue has been approved(bug) or Championed (feature/proposal)
  • Has tests (if omitted, state reason in description)
  • Has samples (if omitted, state reason in description)
  • Rebased on top of main at time of PR
  • Changes adhere to coding standard
  • Documentation created or updated: https://github.com/MicrosoftDocs/CommunityToolkit/pulls

Additional information

Unsure how to add tests - never done testing before.
I think "Has samples" is good/checked, as the issue has a sample it can be tested against.
Honestly I dont understand the comment behind documentation created or updated.

brminnick
brminnick previously approved these changes Nov 14, 2024
Copy link
Collaborator

@brminnick brminnick left a comment

Choose a reason for hiding this comment

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

@brminnick brminnick enabled auto-merge (squash) November 14, 2024 19:36
@matt-bartholomew
Copy link
Author

matt-bartholomew commented Nov 14, 2024 via email

@matt-bartholomew
Copy link
Author

matt-bartholomew commented Nov 14, 2024

@brminnick Going to be honest - this is my first time contributing to opensource stuff and to the communitytoolkit. What's the timeline between this change getting approved, merged, and then actually published in an updated package? I don't know if that information is public anywhere, I just can't find it.

@matt-bartholomew
Copy link
Author

@brminnick Currently failing 3/6 statuses/checks. Not sure that these failures are anything within my control/due to my changes. Can you have a look and just let me know? Thanks!

@bijington
Copy link
Contributor

Thank you for the submission.

The pipeline errors don't appear to be caused by your changes. Well sort those at some point. We've been trying to ship .NET 9.0 support so a release will likely fall on when that gets completed.

We don't have strict timelines for this stuff. Just as and when there is enough to ship

auto-merge was automatically disabled November 15, 2024 14:33

Head branch was pushed to by a user without write access

@pictos
Copy link
Member

pictos commented Nov 18, 2024

@matt-bartholomew can you take at your end and see if the project builds?

@pictos pictos closed this Nov 18, 2024
@pictos pictos reopened this Nov 18, 2024
@matt-bartholomew
Copy link
Author

@matt-bartholomew can you take at your end and see if the project builds?

@pictos I built and tested on my end on windows using the sample app from the linked issue. I had no issue running or anything like that.

@pictos
Copy link
Member

pictos commented Nov 18, 2024

@matt-bartholomew are you on .net 8 or 9? Just to confirm if the issue is on our pipeline

@matt-bartholomew
Copy link
Author

@matt-bartholomew are you on .net 8 or 9? Just to confirm if the issue is on our pipeline

@pictos 8.

@matt-bartholomew
Copy link
Author

@pictos @brminnick @bijington I don't think some of these pipeline failures are due to anything I introduced. Can one of you look and let me know? When would I be able to see this in a release?

@pictos
Copy link
Member

pictos commented Dec 3, 2024

@matt-bartholomew no worries about it. PRs will not be merged until we figure out .NET 9 support

@bijington
Copy link
Contributor

And we should be very close now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Cancel being called on a disposed CancellationTokenSource
4 participants