Skip to content

All semaphor all the time#8755

Merged
scbedd merged 9 commits intomainfrom
semaphoreslim
Aug 1, 2024
Merged

All semaphor all the time#8755
scbedd merged 9 commits intomainfrom
semaphoreslim

Conversation

@scbedd
Copy link
Copy Markdown
Member

@scbedd scbedd commented Aug 1, 2024

This cuts out all lock usage for await SemaphorSlim.WaitAsync() to synchronize resource access.

Originally, we had 3 possible locks

  • object SessionSanitizerLock
  • Session.Entries
  • object Session.SanitizerLock

This moves to using a semaphore for these. Unfortunately this has the impact of forcing async in a lot of places that didn't use it before, so I've got some test pain to resolve as a result.

@mikeharder I'm very pleased with this one!

image
image

@scbedd scbedd self-assigned this Aug 1, 2024
@scbedd scbedd changed the title All semaphor all the time [do not merge] All semaphor all the time Aug 1, 2024
@scbedd scbedd changed the title [do not merge] All semaphor all the time All semaphor all the time Aug 1, 2024
@scbedd scbedd marked this pull request as ready for review August 1, 2024 21:42
@scbedd scbedd requested review from benbp and mikeharder as code owners August 1, 2024 21:42
Copy link
Copy Markdown
Member

@mikeharder mikeharder left a comment

Choose a reason for hiding this comment

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

I agree this is better than synchronous locking. You generally want to use async as much as possible inside asp.net apps.

However, it is still susceptible to deadlocks and/or race conditions, if we don't have the locking semantics/ordering exactly correct. So if this doesn't fix all the issues, we might need to take a closer look at the algorithms we are using,.

@scbedd scbedd merged commit 0ae9770 into main Aug 1, 2024
@scbedd scbedd deleted the semaphoreslim branch August 1, 2024 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

2 participants