Skip to content

Don't release SemaphoreSlim when it is canceled #12818

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Aug 14, 2019
Merged

Don't release SemaphoreSlim when it is canceled #12818

merged 5 commits into from
Aug 14, 2019

Conversation

ajaybhargavb
Copy link
Contributor

Fixes #11686

We don't want to reuse the semaphoreslim when it is cancelled. So moving it out of try block so it doesn't try to release it.

@Pilchie Pilchie added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Aug 2, 2019
@ajaybhargavb
Copy link
Contributor Author

🆙 📅

@ajaybhargavb ajaybhargavb force-pushed the ajbaaska/semaphore-slim branch from d176d69 to 965f990 Compare August 2, 2019 20:44
pranavkm
pranavkm reviewed Aug 2, 2019
View reviewed changes
src/Components/Components/src/Dispatcher.cs
{
if (!CheckAccess())
{
throw new InvalidOperationException(
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a super fan of this API. Telling the user to do this isn't helpful all the time - for example, user code is not involved in the CircuitHost where this is used, so throwing an exception stating they should do something when it's our bug doesn't seem right.

Could we write something specific in the host instead?

Copy link
Member

Choose a reason for hiding this comment

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

It won't happen though.

We could do extra work to have a worse error message for a case that should never happen. Or we can call the API that does what we want, with the expectation that it won't happen.

@ajaybhargavb ajaybhargavb changed the title Don't release SemaphoreSlim when it is cancelled Don't release SemaphoreSlim when it is canceled Aug 2, 2019
@ajaybhargavb ajaybhargavb force-pushed the ajbaaska/semaphore-slim branch from 965f990 to 2414c7f Compare August 5, 2019 17:40
@mkArtakMSFT mkArtakMSFT added area-blazor Includes: Blazor, Razor Components tell-mode Indicates a PR which is being merged during tell-mode and removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels Aug 6, 2019
@ajaybhargavb ajaybhargavb force-pushed the ajbaaska/semaphore-slim branch from c6ff794 to 9ac7437 Compare August 7, 2019 19:34
@pranavkm pranavkm self-assigned this Aug 9, 2019
@pranavkm pranavkm force-pushed the ajbaaska/semaphore-slim branch from 9ac7437 to 7418fab Compare August 9, 2019 17:49
@ajaybhargavb
Copy link
Contributor Author

@aspnet-hello triage https://dev.azure.com/dnceng/public/_build/results?buildId=302894&view=logs

@aspnet-hello
Copy link

This comment was made automatically. If there is a problem contact [email protected].

I've triaged the above build. I've created/commented on the following issue(s)
https://github.com/aspnet/AspNetCore-Internal/issues/2866

@ajaybhargavb ajaybhargavb force-pushed the ajbaaska/semaphore-slim branch 4 times, most recently from 2cbacbb to 4e1332c Compare August 12, 2019 20:12
@ajaybhargavb ajaybhargavb force-pushed the ajbaaska/semaphore-slim branch from 4e1332c to 49b5550 Compare August 12, 2019 22:24
@rynowak
Copy link
Member

rynowak commented Aug 14, 2019

@ajaybhargavb - is this is blocked on anything? If there are unrelated tests that are failing, mark as flaky and move on 😁

@ajaybhargavb ajaybhargavb force-pushed the ajbaaska/semaphore-slim branch from 49b5550 to ae4f195 Compare August 14, 2019 16:35
@ajaybhargavb ajaybhargavb merged commit c605d6c into release/3.0 Aug 14, 2019
@ghost ghost deleted the ajbaaska/semaphore-slim branch August 14, 2019 19:40
@github-actions github-actions bot locked and limited conversation to collaborators Dec 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pranavkm pranavkm pranavkm left review comments

@rynowak rynowak rynowak approved these changes

@SteveSandersonMS SteveSandersonMS Awaiting requested review from SteveSandersonMS

@BrennanConroy BrennanConroy Awaiting requested review from BrennanConroy

Assignees

@pranavkm pranavkm

Labels
area-blazor Includes: Blazor, Razor Components tell-mode Indicates a PR which is being merged during tell-mode
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ajaybhargavb @aspnet-hello @rynowak @pranavkm @Pilchie @mkArtakMSFT