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

Potential fix for #897. Use monitor for recursive mutual exclusion. #898

Merged
merged 1 commit into from Nov 28, 2019

Conversation

@ioquatix
Copy link
Contributor

ioquatix commented Oct 24, 2019

Remove without_mutex option as it's now irrelevant.

Remove without_mutex option as it's now irrelevant.
@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 24, 2019

Coverage Status

Coverage decreased (-0.002%) to 99.967% when pulling e1852e0 on ioquatix:thread-safety into 3ca8a3b on RubyMoney:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 24, 2019

Coverage Status

Coverage decreased (-0.004%) to 99.965% when pulling e1852e0 on ioquatix:thread-safety into 3ca8a3b on RubyMoney:master.

2 similar comments
@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 24, 2019

Coverage Status

Coverage decreased (-0.004%) to 99.965% when pulling e1852e0 on ioquatix:thread-safety into 3ca8a3b on RubyMoney:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 24, 2019

Coverage Status

Coverage decreased (-0.004%) to 99.965% when pulling e1852e0 on ioquatix:thread-safety into 3ca8a3b on RubyMoney:master.

@@ -88,20 +89,18 @@ def transaction(&block)
# puts [iso_from, iso_to, rate].join
# end
def each_rate(&block)

This comment has been minimized.

Copy link
@sholden

sholden Oct 28, 2019

No need to accept &block any more if you're just going to yield instead of passing the block to the Enumerator.

Copy link
Contributor

antstorm left a comment

Couple of comments, but overall looks great! Thank you very much for the PR 👍

end

private

attr_reader :index, :options

This comment has been minimized.

Copy link
@antstorm

antstorm Nov 10, 2019

Contributor

any reason for removing options?

This comment has been minimized.

Copy link
@ioquatix

ioquatix Nov 11, 2019

Author Contributor

It was private and not used by anything.

This comment has been minimized.

Copy link
@ioquatix

ioquatix Dec 2, 2019

Author Contributor

@prognostikos Thanks for the cross-reference. Unfortunately for users of that code, that method is completely thread unsafe. The best solution would be to remove it entirely.

This comment has been minimized.

Copy link
@prognostikos

prognostikos Dec 2, 2019

OK, I've opened an issue over there.

[self.class, options, index]
@guard.synchronize do
return [self.class, @options, @rates.dup]
end
end

# Wraps block execution in a thread-safe transaction
def transaction(&block)

This comment has been minimized.

Copy link
@antstorm

antstorm Nov 10, 2019

Contributor

Since you're no longer using it directly, there's need for an explicit &block argument

@@ -88,20 +89,18 @@ def transaction(&block)
# puts [iso_from, iso_to, rate].join
# end
def each_rate(&block)
enum = Enumerator.new do |yielder|
index.each do |key, rate|
return to_enum(:each_rate) unless block_given?

This comment has been minimized.

Copy link
@antstorm

antstorm Nov 10, 2019

Contributor

this is great 👍

@antstorm antstorm merged commit 57c3da8 into RubyMoney:master Nov 28, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ioquatix ioquatix deleted the ioquatix:thread-safety branch Nov 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.