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

Fix circular dependency between AtomicReference and Synchronization::Object #982

Merged
merged 8 commits into from
Jan 11, 2023
Merged

Fix circular dependency between AtomicReference and Synchronization::Object #982

merged 8 commits into from
Jan 11, 2023

Conversation

eregon
Copy link
Collaborator

@eregon eregon commented Jan 8, 2023 •
edited
Loading

  • Do not depend on Synchronization::LockableObject for MutexAtomic{Reference,Fixnum,Boolean}
  • Extract safe initialization and use it for MutexAtomic{Reference,Fixnum,Boolean}

TODO:

  • Check what gets defined by the JRuby extension, might need more require 'concurrent/utility/native_extension_loader' # load native parts first for JRuby
  • Audit usages of java_extensions_loaded?

eregon added 5 commits January 8, 2023 21:02
@eregon
Fix circular dependency between AtomicReference and Synchronization::…
8ca93a8 
…Object

* Specifically, the cycle is:
  Synchronization::Object -> AtomicReference (through generated code for #__initialize_atomic_fields__ used by #initialize)
  AtomicReference -> MutexAtomicReference (only for cruby no exts and unknown ruby)
  MutexAtomicReference -> Synchronization::LockableObject
  Synchronization::LockableObject -> MutexLockableObject, JRubyLockableObject, MonitorLockableObject
  MutexLockableObject -> AbstractLockableObject
  AbstractLockableObject -> Synchronization::Object

  As an example AtomicMarkableReference -> Synchronization::Object uses it
  and so the requires of spec/concurrent/atomic/atomic_markable_reference_spec.rb
  cannot be fixed before this.
* This also avoids allocating a ConditionVariable for MutexAtomicReference.
@eregon
Do not depend on Synchronization::LockableObject for MutexAtomicBoole…
76882e9 
…an and MutexAtomicFixnum

* Similar to MutexAtomicReference.
* Avoids allocating an extra ConditionVariable.
* Ensure the native extension version is used on CRuby in specs.
@eregon
Extract safe initialization and use it for MutexAtomic{Reference,Fixn…
0511d48 
…um,Boolean}

* The previous logic risked a thread seeing @__Lock__ being nil
  if the object was published racily.
@eregon
Be more explicit that it's only included once
81678dc 
* Avoids relying on the precise semantics of #extend.
@eregon
Rename to @Lock for consistency since it's a final ivar
973f1e6
@eregon eregon force-pushed the fix-cycle-AtomicReference-Synchronization-Object branch from 54a9975 to 973f1e6 Compare January 8, 2023 20:31
@eregon eregon force-pushed the fix-cycle-AtomicReference-Synchronization-Object branch from b574b30 to f3d8a89 Compare January 11, 2023 15:49
@eregon eregon mentioned this pull request Jan 11, 2023
Merged
4 tasks
@eregon eregon merged commit eac0237 into ruby-concurrency:master Jan 11, 2023
@eregon eregon deleted the fix-cycle-AtomicReference-Synchronization-Object branch January 11, 2023 16:12
@eregon eregon linked an issue Jan 23, 2023 that may be closed by this pull request
Officially support requiring subparts of concurrent-ruby, e.g., require 'concurrent/map' #934
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Officially support requiring subparts of concurrent-ruby, e.g., require 'concurrent/map'
1 participant
@eregon