Skip to content

Dispose CancellationTokenSource #49796

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

Merged
merged 1 commit into from
Aug 2, 2023

Conversation

alex-inftx
Copy link
Contributor

we can dispose CancellationTokenSource after use

@ghost ghost added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates area-runtime community-contribution Indicates that the PR has been added by a community member labels Aug 2, 2023
@ghost
Copy link

ghost commented Aug 2, 2023

Thanks for your PR, @alex-inftx. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@ghost ghost added this to the 6.0.x milestone Aug 2, 2023
@ghost
Copy link

ghost commented Aug 2, 2023

Hi @alex-inftx. If this is not a tell-mode PR, please make sure to follow the instructions laid out in the servicing process document.
Otherwise, please add tell-mode label.

@gfoidl
Copy link
Member

gfoidl commented Aug 2, 2023

@alex-inftx please change the target branch of this PR to be main (and not release/6.0).

@sharwell sharwell changed the base branch from release/6.0 to main August 2, 2023 12:39
@sharwell sharwell changed the base branch from main to release/6.0 August 2, 2023 12:40
@sharwell
Copy link
Contributor

sharwell commented Aug 2, 2023

I changed the base branch to main not realizing that release/6.0 hasn't merged into main yet. Sorry for the noise.

@mgravell
Copy link
Member

mgravell commented Aug 2, 2023

I changed the base branch to main not realizing that release/6.0 hasn't merged into main yet. Sorry for the noise.

Still looks like 6.0 unless I'm misreading; if you click edit, the target branch should become editable:

image

However, if it starts freaking out, it may be easier to redo the branch against main.

as an aside: there is no "yet" here - 6.0 is a maintenance branch; it'll stay independent until some future hypothetical point when it simply gets nuked

@sebastienros sebastienros changed the base branch from release/6.0 to main August 2, 2023 15:20
@sebastienros
Copy link
Member

I changed the base branch but now there are lots of conflicts. Might be easier to create a new PR.

@sebastienros sebastienros changed the base branch from main to release/6.0 August 2, 2023 15:21
@mgravell
Copy link
Member

mgravell commented Aug 2, 2023

image

yeah, that would be the "freaking out" I was hoping to avoid :) in theory it should be possible to rebase and force push, but for a 2 line change, starting a clean branch from main is probably much much simpler

@MichaelSimons MichaelSimons removed the request for review from a team August 2, 2023 15:52
@alex-inftx alex-inftx changed the base branch from release/6.0 to main August 2, 2023 16:04
@alex-inftx alex-inftx closed this Aug 2, 2023
@alex-inftx alex-inftx reopened this Aug 2, 2023
@ghost
Copy link

ghost commented Aug 2, 2023

Hi @alex-inftx. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@alex-inftx
Copy link
Contributor Author

@alex-inftx please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@dotnet-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@dotnet-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@dotnet-policy-service agree company="Microsoft"

Contributor License Agreement

@dotnet-policy-service agree company="infotecs"

@alex-inftx
Copy link
Contributor Author

image

yeah, that would be the "freaking out" I was hoping to avoid :) in theory it should be possible to rebase and force push, but for a 2 line change, starting a clean branch from main is probably much much simpler

It's done [rebase and+force push] )

Copy link
Member

@mgravell mgravell 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 to me, and should clean up a timer callback 👍

@alex-inftx
Copy link
Contributor Author

This looks good to me, and should clean up a timer callback 👍
thank you!
@mgravell , should I create a new PRs to place changes to release/6.0 and release/7.0 ?

@mgravell
Copy link
Member

mgravell commented Aug 2, 2023

@alex-inftx IMO no this doesn't warrant backport. Yes, it may be more efficient, but: the feature isn't fundamentally broken or dangerous "as is". Others may disagree, but I vote "no"

@mkArtakMSFT mkArtakMSFT removed the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Aug 2, 2023
@adityamandaleeka adityamandaleeka merged commit 22b8ebb into dotnet:main Aug 2, 2023
@adityamandaleeka
Copy link
Member

Thanks @alex-inftx. I agree with @mgravell that this doesn't need to be backported.

@ghost ghost modified the milestones: 6.0.x, 8.0-rc1 Aug 2, 2023
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions 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.

8 participants