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

Consider removal of threadsafe memoized helpers #2373

Closed
JoshCheek opened this issue Jan 18, 2017 · 3 comments
Closed

Consider removal of threadsafe memoized helpers #2373

JoshCheek opened this issue Jan 18, 2017 · 3 comments

Comments

@JoshCheek
Copy link
Contributor

JoshCheek commented Jan 18, 2017

The discussion about the threadsafe memoized helpers, initially introduced in #1858. I'm opening the issue because rspec/rspec#14 has shifted to this topic, which merits its own issue. If we do decide to remove it, then that issue can be closed.

I spent a couple of days once writing up an argument that we should remove the feature (it's a strategy I use to force my brain to thoroughly consider a topic). It didn't wind up being clearly the correct decision, so I didn't post it and instead requested that this pull request be re-attempted. I looked pretty hard for this file and can't find it.

One hesitation I have is that the feature is useful on Rubinius and JRuby where there is no GIL and race conditions can occur in the memoized helpers. However in this comment, @myronmarston says "you're the first user to ever report a threadsafety problem" which is evidence that in practice it is rarely an issue. It's not the most compelling metric because when I experienced it, I spent ~8 hours trying to find the bug in my code before I realized it was a race condition in the memoized helpers. However, even if people are hitting the race condition, they're not opening issues in RSpec, and people are definitely opening issues when they hit deadlocks due to the threadsafety. So the cost of maintenance is higher with the feature, even if is more useful than the number of issues would imply.

Another downside of the feature is that understanding threaded code requires one to go through a gauntlet of brain desecrating confusion. I assume this makes working with this code very formidable which causes it to become a silo. As evidence of this, note that I've been requested to analyze user issues on a number of occasions (I know I should go find them and link it, but I'd like to get back to looking for my unpublished consideration of its removal). This is probably a much higher cost than is immediately apparent since MRI's GIL shields users from the painful experience they would need in order to understand the contexts.

Ultimately, the cost of maintenance falls mostly on the RSpec team and I shoulder only a small amount of it. So despite being too ambivalent to currently have an opinion, if that maintenance cost is too high, then that reason alone would seem sufficient to remove it.

@xaviershay
Copy link
Member

Thanks for writing up your thoughts. I didn't realise that this was purely an opt-in feature (at the moment - we were considering making it default.)

Wondering what hooks be needed to make this a plugin ... will take a look at that angle this morning.

@xaviershay
Copy link
Member

Investigating my hypothetical above: if RSpec::Core::MemoizedHelpers#__init_memoized was made polymorphic, with the selection method made semi-public, I'm pretty sure that would enable thread-
safety to be provided in a plugin. Then we can provide it with all the disclaimers/YMMV we want.

Alternatively, we could have a config.memoize_store option do achieve the same, but I don't particularly like that. Seems too detailed for public configuration.

(Implicit in this is my assumption that things in core should "just work" and come with an expectation of support and ease-of-use, and that thread safe let doesn't currently meet that bar.)

@JoshCheek
Copy link
Contributor Author

Ahh, I clearly misunderstood what you were saying in the other issue.

Alternatively, we could have a config.memoize_store option do achieve the same, but I don't particularly like that. Seems too detailed for public configuration.

I'd have to look at it again to be able to provide anything helpful here. Happy to do that if you'd like. Since it seems I already misunderstood what you were saying, I'll close this issue. I'd make a new one for making it the default / removing the configuration, but given I got this one incorrect, it's probably best for you to make it, ping me in it if you want me to take a look :)

(Implicit in this is my assumption that things in core should "just work" and come with an expectation of support and ease-of-use, and that thread safe let doesn't currently meet that bar.)

Aye ^_^

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

No branches or pull requests

2 participants