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

Fix ReentrantMutex locking inside Fiber spec for ruby-3.2 #553

Closed
wants to merge 1 commit into from

Conversation

ojab
Copy link

@ojab ojab commented Nov 18, 2022

It raises ThreadError since ruby/ruby#6680

@ojab
Copy link
Author

ojab commented Nov 18, 2022

First-time contributors need a maintainer to approve running workflows.

Sending to review, specs are green locally with ruby-3.2-preview3

@ojab
Copy link
Author

ojab commented Nov 18, 2022

Also, should reach here is copypasted from existing spec, shouldn't it be should not reach here?

@pirj pirj force-pushed the fixup_reentrant_mutex_on_3.2 branch from 66ab781 to 269599f Compare November 19, 2022 07:53
@pirj
Copy link
Member

pirj commented Nov 19, 2022

The old spec didn't fail with ruby 3.2.0dev (2022-10-26T15:29:12Z master fa0adbad92), and it doesn't fail now with ruby 3.2.0dev (2022-11-17T18:58:56Z master 0446d961a0).
The breaking change in Ruby was made between those runs.
We skip sub-builds for ruby-head, so it's not possible to confirm the failure from rspec-mocks build.

I intend to:

  • stash the changes temporarily
  • reproduce the failure
  • get the changes back

@pirj
Copy link
Member

pirj commented Nov 19, 2022

A failing 3.2 build 👍

@pirj pirj force-pushed the fixup_reentrant_mutex_on_3.2 branch from 5345377 to 5de969f Compare November 19, 2022 08:14
Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

Thank you!

@pirj pirj requested a review from JonRowe November 19, 2022 08:18
@JonRowe
Copy link
Member

JonRowe commented Nov 19, 2022

Realistically this should be a pending spec on Ruby 3.2 as its something we should look to fix... (Its not expected to error, it just does)...

Copy link
Contributor

@eregon eregon left a comment

Choose a reason for hiding this comment

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

Looks good to me, this just adapts the test to the new exception/behavior in CRuby

@JonRowe
Copy link
Member

JonRowe commented Jan 6, 2023

I've been working on reworking the spec to run, I don't think its a change in behaviour it's just the way of testing it that broke?

@eregon
Copy link
Contributor

eregon commented Jan 6, 2023

It's a behavior change in Ruby, the deadlock is detected and so it doesn't hang.
I think it's difficult to design a test that works with both and is still readable, so I think this is a good solution.

An alternative would be to use a Fiber scheduler, then it would behave as before, since with a scheduler it makes sense to wait. A Fiber scheduler is probably the reason we needed to fix RSpec ReentrantMutex. Would need to make a dummy Fiber scheduler for tests, it's probably fairly short if only used for this example.

@eregon
Copy link
Contributor

eregon commented Jan 6, 2023

#501 is the original issue

@JonRowe
Copy link
Member

JonRowe commented Jan 7, 2023

I'm closing this for now, we currently skip this on Rubies were its broken, I'd like to fix this spec rather than a no op expect an error (probably needing a fiber scheduler) especially as on my machine its definielty a broken spec (the raise leaks rather than a deadlock)

@JonRowe JonRowe closed this Jan 7, 2023
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

Successfully merging this pull request may close these issues.

None yet

4 participants