Skip to content

ReadWriteLock donated by @alexdowad #259

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

Merged
merged 8 commits into from
Mar 7, 2015
Merged

ReadWriteLock donated by @alexdowad #259

merged 8 commits into from
Mar 7, 2015

Conversation

jdantonio
Copy link
Member

Merges ReadWriteLock donated by @alexdowad in #146.

  • Copied the source file to the lib/concurrent/atomic directory
  • Moved the class under the Concurrent namespace
  • Moved the stress tests to examples/benchmark_read_write_lock.rb
  • Updated the stress tests to use OptionsParser
  • Created a spec file

To-do

  • Yardoc
  • Tests
  • Refactor
  • Changelog
  • Readme

@jdantonio jdantonio added in progress enhancement Adding features, adding tests, improving documentation. labels Mar 4, 2015
@jdantonio jdantonio self-assigned this Mar 4, 2015
@jdantonio jdantonio added this to the 0.9.0 Release milestone Mar 4, 2015
@alexdowad
Copy link
Contributor

@jdantonio, thank you very much for cleaning up my code!

@jdantonio
Copy link
Member Author

@alexdowad No clean up was necessary. Your code is awesome! All I had to do was integrate it into the project. Thank you very much for donating it!

result = yield
release_read_lock
result
end
Copy link
Member

Choose a reason for hiding this comment

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

better to wrap release_read_lock in ensure block, to prevent leaving it locked

Copy link
Member Author

Choose a reason for hiding this comment

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

My next commit will have an ensure block that releases the lock but is also sensitive to errors raised before the lock is acquired (such as when the maximum number of readers/writers is reached).

Copy link
Member

Choose a reason for hiding this comment

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

right, it has to be:

def with_read_lock
  acquire_read_lock
  begin
    yield
  ensure
    release_read_lock
  end
end

not just ensure at the end, good catch

@pitr-ch
Copy link
Member

pitr-ch commented Mar 4, 2015

@alexdowad Thanks for the code!

@pitr-ch
Copy link
Member

pitr-ch commented Mar 5, 2015

The complex conditions could be extracted to methods to improve readability. E.g.

def readers(c = @count.value)
  c & MAX_READERS
end

def waiting_writer?(c = @counter.value)
  c >= WAITING_WRITER
end

# etc

@jdantonio
Copy link
Member Author

I believe this PR is ready to merge. It has nearly 100% code coverage, full yardoc, better exception handling, a few readability updates (methods for some of the bitwise operations used in conditionals), and the stress tests still pass with comparable performance.

jdantonio added a commit that referenced this pull request Mar 7, 2015
@jdantonio jdantonio merged commit 2859060 into master Mar 7, 2015
@jdantonio jdantonio deleted the read_write_lock branch March 7, 2015 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Adding features, adding tests, improving documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants