Replace `ActiveSupport::Concurrency::Latch` with `Concurrent::CountDownLatch` from concurrent-ruby. #20866

Merged
merged 1 commit into from Jul 14, 2015

Conversation

Projects
None yet
2 participants
@jdantonio
Contributor

jdantonio commented Jul 13, 2015

This update was suggested by @tenderlove in a discussion on one of his PRs to concurrent-ruby.

The concurrent-ruby gem is a toolset containing many concurrency utilities. Many of these utilities include runtime-specific optimizations when possible. Rather than clutter the Rails codebase with concurrency utilities separate from the core task, such tools can be superseded by similar tools in the more specialized gem. This commit replaces ActiveSupport::Concurrency::Latch with Concurrent::CountDownLatch, which is functionally equivalent.

@tenderlove

This comment has been minimized.

Show comment
Hide comment
@tenderlove

tenderlove Jul 13, 2015

Member

@jdantonio thanks. I don't think this is enough though. We have runtime requirements for the countdown latch. Can you update the gemspecs that need concurrent-ruby to depend on it? I think only actionpack needs it.

Also we may want to consider adding a deprecation warning for the countdown latch that's in Rails (though since this is Rails 5, and I don't think many people use our latch I'm not sure we need that).

Member

tenderlove commented Jul 13, 2015

@jdantonio thanks. I don't think this is enough though. We have runtime requirements for the countdown latch. Can you update the gemspecs that need concurrent-ruby to depend on it? I think only actionpack needs it.

Also we may want to consider adding a deprecation warning for the countdown latch that's in Rails (though since this is Rails 5, and I don't think many people use our latch I'm not sure we need that).

@jdantonio

This comment has been minimized.

Show comment
Hide comment
@jdantonio

jdantonio Jul 13, 2015

Contributor

Sorry I missed the runtime requirement. I thought it was only used in tests. I'll update ASAP.

Contributor

jdantonio commented Jul 13, 2015

Sorry I missed the runtime requirement. I thought it was only used in tests. I'll update ASAP.

@tenderlove

This comment has been minimized.

Show comment
Hide comment
@tenderlove

tenderlove Jul 13, 2015

Member

@jdantonio thanks so much! Also, if you don't mind, could you check our uses of the thread_safe gem? AFAICT, concurrent-ruby supersedes it, and I'd rather just switch thread_safe to concurrent-ruby if that's possible.

Member

tenderlove commented Jul 13, 2015

@jdantonio thanks so much! Also, if you don't mind, could you check our uses of the thread_safe gem? AFAICT, concurrent-ruby supersedes it, and I'd rather just switch thread_safe to concurrent-ruby if that's possible.

@jdantonio

This comment has been minimized.

Show comment
Hide comment
@jdantonio

jdantonio Jul 13, 2015

Contributor

@tenderlove We have not merged thread_safe into concurrent-ruby yet, just ruby-atomic. We want to get c-r 1.0 out before we merge thread_safe.

Contributor

jdantonio commented Jul 13, 2015

@tenderlove We have not merged thread_safe into concurrent-ruby yet, just ruby-atomic. We want to get c-r 1.0 out before we merge thread_safe.

@tenderlove

This comment has been minimized.

Show comment
Hide comment
@tenderlove

tenderlove Jul 13, 2015

Member

I see. Nevermind about thread_safe then. :)

Member

tenderlove commented Jul 13, 2015

I see. Nevermind about thread_safe then. :)

Replaced `ActiveSupport::Concurrency::Latch` with concurrent-ruby.
The concurrent-ruby gem is a toolset containing many concurrency
utilities. Many of these utilities include runtime-specific
optimizations when possible. Rather than clutter the Rails codebase with
concurrency utilities separate from the core task, such tools can be
superseded by similar tools in the more specialized gem. This commit
replaces `ActiveSupport::Concurrency::Latch` with
`Concurrent::CountDownLatch`, which is functionally equivalent.
@jdantonio

This comment has been minimized.

Show comment
Hide comment
@jdantonio

jdantonio Jul 13, 2015

Contributor

@tenderlove I put ActiveSupport::Concurrency::Latch back but it now extends Concurrent::CountDownLatch and gives a deprecation warning.

I removed c-r from Gemfile and put the c-r dependency in the gemspec for activesupport (since it now uses CountDownLatch directly). Both actionpack and activerecord use CountDownLatch in their tests but both should pick up the dependency through activesupport.

I couldn't find any place in actionpack that used the original Latch and the tests all passed on the original commit. I added c-r to its gemspec anyway, at your suggestion.

Contributor

jdantonio commented Jul 13, 2015

@tenderlove I put ActiveSupport::Concurrency::Latch back but it now extends Concurrent::CountDownLatch and gives a deprecation warning.

I removed c-r from Gemfile and put the c-r dependency in the gemspec for activesupport (since it now uses CountDownLatch directly). Both actionpack and activerecord use CountDownLatch in their tests but both should pick up the dependency through activesupport.

I couldn't find any place in actionpack that used the original Latch and the tests all passed on the original commit. I added c-r to its gemspec anyway, at your suggestion.

@jdantonio jdantonio referenced this pull request in ruby-concurrency/concurrent-ruby Jul 14, 2015

Merged

Removed unnecessary extension warnings. #371

tenderlove added a commit that referenced this pull request Jul 14, 2015

Merge pull request #20866 from jdantonio/countdown-latch
Replace `ActiveSupport::Concurrency::Latch` with `Concurrent::CountDownLatch` from concurrent-ruby.

@tenderlove tenderlove merged commit 468a55b into rails:master Jul 14, 2015

@jdantonio jdantonio deleted the jdantonio:countdown-latch branch Jul 14, 2015

@jdantonio jdantonio referenced this pull request in ruby-concurrency/concurrent-ruby Jul 30, 2015

Merged

Import `thread_safe` gem #386

@sonalkr132 sonalkr132 referenced this pull request in rubygems/rubygems.org May 10, 2018

Merged

Persist records before uploading to s3 #1723

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment