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.

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

Kotlin coroutines ReadWriteMutex #671

Merged
merged 15 commits into from Oct 9, 2020
Merged

Kotlin coroutines ReadWriteMutex #671

merged 15 commits into from Oct 9, 2020

Conversation

frett
Copy link
Contributor

@frett frett commented Oct 7, 2020

Unfortunately Kotlin Coroutines doesn't have a native ReadWriteMutex yet. This implements one with basic functionality as a stop-gap solution until they provide an official one.

a ReadWriteMutex should function similarly to a ReadWriteReentrantLock.

  • write access is exclusive, so only a single process can be writing at a time.
  • While a process is writing no other processes can be reading.
  • read access is shared, so any number of readers can share the lock at once.
  • while any readers exist, a writer will be blocked

This implementation doesn't address fairness for writers, so it is possible for readers to starve out waiting writers. but my current use-cases for this lock uses write access to perform background maintenance, and the background maintenance isn't critical for functionality, so it's alright if the writers are starved.

Relevant coroutines issue: Kotlin/kotlinx.coroutines#94

@frett frett marked this pull request as ready for review October 7, 2020 21:47
@frett frett requested a review from mattdrees October 7, 2020 21:47
@codecov
Copy link

codecov bot commented Oct 7, 2020

Codecov Report

Merging #671 into master will increase coverage by 0.16%.
The diff coverage is 80.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #671      +/-   ##
============================================
+ Coverage     35.48%   35.64%   +0.16%     
  Complexity      485      485              
============================================
  Files           203      205       +2     
  Lines          4312     4337      +25     
  Branches        744      748       +4     
============================================
+ Hits           1530     1546      +16     
- Misses         2559     2568       +9     
  Partials        223      223              
Impacted Files Coverage Δ Complexity Δ
...android/common/kotlin/coroutines/ReadWriteMutex.kt 80.00% <80.00%> (ø) 0.00 <0.00> (?)
...i/gto/android/common/kotlin/coroutines/MutexMap.kt 0.00% <0.00%> (ø) 0.00% <0.00%> (?%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b13da3...8b9702d. Read the comment docs.

Copy link
Member

@mattdrees mattdrees left a comment

Choose a reason for hiding this comment

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

Some suggestions, but only one of them is relatively important. This looks good to me, otherwise.

we need to make sure we use the same owner between locking and unlocking the write lock from within multiple read locks.
`stateMutex` is purely used internally, so we don't need to pass in an arbitrary owner
@frett
Copy link
Contributor Author

frett commented Oct 9, 2020

Thanks for the thorough review @mattdrees you definitely caught some things that I missed

@frett frett merged commit 1b800f5 into master Oct 9, 2020
@frett frett deleted the readWriteMutex branch October 9, 2020 15:17
@mattdrees
Copy link
Member

You're welcome!

And your fixes look good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants