-
Notifications
You must be signed in to change notification settings - Fork 934
Use consistent locking for async and sync code #2147
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
Conversation
@@ -150,9 +150,6 @@ | |||
transformation: | |||
configureAwaitArgument: false | |||
localFunctions: true | |||
asyncLock: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will most likely remove this option in the future as adding an additional field for async locking allows two threads to execute the "same code" simultaneously.
} | ||
} | ||
|
||
private struct Releaser : IDisposable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no point making this struct if it is a private. It will be boxed and leave in the heap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, I changed it to a sealed class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did some perf testing. The fastest (and free of additional allocations) method is to return the struct. The second place is class (does not matter if it is returned as class or as IDisposable
) and third place is a struct boxed to IDisposable
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the code to use the fastest option. We could modify the struct to be readonly
, but it requires C# language to be upgraded to 7.2
and I am not sure if it would perform any better. Also, I didn't apply the change for AsyncLock
, as it would be a breaking change.
badbd90
to
9c1007f
Compare
Rebased for resolving conflicts |
Currently, the async generator is generating separate fields for lock statements that contain an async invocation, which may cause troubles as two threads may simultaneously execute the same code, one for async and one in sync version of the method. Instead of using a lock statement, this PR uses
AsyncLock
for both sync and async locking, which prevents the mentioned problem. In addition,AsyncReaderWriterLock
class was added and used forReadWriteCache
andUpdateTimestampsCache
in order to avoid locking when only read operations are performed.