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.

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

Miscellaneous ReaderWriterLockSlim issues #8710

Closed
kouvel opened this issue Aug 7, 2017 · 4 comments
Closed

Miscellaneous ReaderWriterLockSlim issues #8710

kouvel opened this issue Aug 7, 2017 · 4 comments
Labels
area-System.Threading backlog-cleanup-candidate An inactive issue that has been marked for automated closure. bug no-recent-activity tenet-reliability Reliability/stability related issue (stress, load problems, etc.)

Comments

@kouvel
Copy link
Member

kouvel commented Aug 7, 2017

  • In the TryEnter* functions the conditions checked before the spin loop need to be done inside the spin loop to account for reentrancy due to message pumping during a wait (including Sleep and WaitOne), during which the RW lock state may change.
    • Exception cases should be rechecked upon timeout in case of a state change
  • An exception from GetThreadRWCount(dontAllocate: false) was not being accounted for. Upon OOM, it could lead to not exiting _myLock, hanging subsequent uses of the RW lock
@kouvel kouvel self-assigned this Aug 7, 2017
@kouvel kouvel changed the title ReaderWriterLockSlim has issues with reentrancy due to message pumping Miscellaneous ReaderWriterLockSlim issues Aug 8, 2017
kouvel referenced this issue in kouvel/coreclr Aug 8, 2017
@kouvel
Fix miscellaneous ReaderWriterLockSlim issues
dafa744 
- Stop spinning for _myLock when deprioritized and the timeout expires, as deprioritized means the intended progress cannot be made on the RW lock

Fixes #13254:
- In the TryEnter* functions the conditions checked before the spin loop are now done inside the spin loop to account for reentrancy due to message pumping during a wait (including Sleep and WaitOne), during which the RW lock state may change.
  - Refactored condition checking in the spin loop to do some before and after the loop and some at the top of the loop, to minimize negative perf impact on the common fast paths
  - Exception cases are rechecked upon timeout in case of a state change
- An exception from GetThreadRWCount(dontAllocate: false) is accounted for. Upon OOM, it previously could have led to not exiting _myLock, hanging subsequent uses of the RW lock.
- When attempting to acquire a write lock, if the thread became a waiter and got signaled but failed to acquire the write lock, it now wakes up read waiters as they would otherwise be blocked by the write waiter. This was previously only handled for the case where the wait times out, it is now handled on all paths.
kouvel referenced this issue in kouvel/coreclr Aug 8, 2017
@kouvel
Fix miscellaneous ReaderWriterLockSlim issues
e91a1d6 
- Stop spinning for _myLock when deprioritized and the timeout expires, as deprioritized means the intended progress cannot be made on the RW lock

Fixes #13254:
- In the TryEnter* functions the conditions checked before the spin loop are now done inside the spin loop to account for reentrancy due to message pumping during a wait (including Sleep and WaitOne), during which the RW lock state may change.
  - Refactored condition checking in the spin loop to do some before and after the loop and some at the top of the loop, to minimize negative perf impact on the common fast paths
  - Exception cases are rechecked upon timeout in case of a state change
- An exception from GetThreadRWCount(dontAllocate: false) is accounted for. Upon OOM, it previously could have led to not exiting _myLock, hanging subsequent uses of the RW lock.
- When attempting to acquire a write lock, if the thread became a waiter and got signaled but failed to acquire the write lock, it now wakes up read waiters as they would otherwise be blocked by the write waiter. This was previously only handled for the case where the wait times out, it is now handled on all paths.
kouvel referenced this issue in kouvel/coreclr Aug 20, 2017
@kouvel
Fix miscellaneous ReaderWriterLockSlim issues
87010a0 
- Stop spinning for _myLock when deprioritized and the timeout expires, as deprioritized means the intended progress cannot be made on the RW lock

Fixes #13254:
- In the TryEnter* functions the conditions checked before the spin loop are now done inside the spin loop to account for reentrancy due to message pumping during a wait (including Sleep and WaitOne), during which the RW lock state may change.
  - Refactored condition checking in the spin loop to do some before and after the loop and some at the top of the loop, to minimize negative perf impact on the common fast paths
  - Exception cases are rechecked upon timeout in case of a state change
- An exception from GetThreadRWCount(dontAllocate: false) is accounted for. Upon OOM, it previously could have led to not exiting _myLock, hanging subsequent uses of the RW lock.
- When attempting to acquire a write lock, if the thread became a waiter and got signaled but failed to acquire the write lock, it now wakes up read waiters as they would otherwise be blocked by the write waiter. This was previously only handled for the case where the wait times out, it is now handled on all paths.
@kouvel kouvel removed their assignment Sep 15, 2017
@kouvel
Copy link
Member Author

kouvel commented Jun 1, 2018

Nice to have, but it is not clear that it would have any significant impact, not likely to happen until a significant issue is shown to substantiate the work

@kouvel kouvel closed this as completed Jun 1, 2018
@kouvel
Copy link
Member Author

kouvel commented Mar 4, 2019

https://github.com/dotnet/coreclr/issues/22792

@kouvel kouvel reopened this Mar 4, 2019
@msftgits msftgits transferred this issue from dotnet/coreclr Jan 31, 2020
@msftgits msftgits added this to the Future milestone Jan 31, 2020
@jkotas jkotas added tenet-reliability Reliability/stability related issue (stress, load problems, etc.) and removed reliability labels Feb 4, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 26, 2020
@stephentoub stephentoub removed the untriaged New issue has not been triaged by the area owner label Feb 28, 2020
@kouvel kouvel mentioned this issue Mar 9, 2020
Closed
@kouvel kouvel mentioned this issue Sep 7, 2022
Closed
@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

Due to lack of recent activity, this issue has been marked as a candidate for backlog cleanup. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will undo this process.

This process is part of our issue cleanup automation.

@dotnet-policy-service dotnet-policy-service bot added backlog-cleanup-candidate An inactive issue that has been marked for automated closure. no-recent-activity labels Aug 30, 2024
@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

This issue will now be closed since it had been marked no-recent-activity but received no further activity in the past 14 days. It is still possible to reopen or comment on the issue, but please note that the issue will be locked if it remains inactive for another 30 days.

@dotnet-policy-service dotnet-policy-service bot removed this from the Future milestone Sep 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area-System.Threading backlog-cleanup-candidate An inactive issue that has been marked for automated closure. bug no-recent-activity tenet-reliability Reliability/stability related issue (stress, load problems, etc.)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@stephentoub @maryamariyan @jkotas @msftgits @kouvel