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

Add specs for ractors access to module instance variables #1043

Conversation

dushyantss
Copy link
Contributor

This PR adds specs for Feature #17592

From:

Non main-Ractors can get instance variables (ivars) of classes/modules
if ivars refer to shareable objects.
[Feature #17592]

@dushyantss dushyantss marked this pull request as draft May 31, 2023 23:36
@dushyantss
Copy link
Contributor Author

dushyantss commented Jun 1, 2023

The specs for ObjectSpace._id2ref are failing because they behave differently in single vs multi ractor environments.

  • On multi-ractor mode, if the object is not shareable, it raises
  • RangeError.

I have not been able to find a method in RubyLand(found rb_multi_ractor_p in the C code, but no way to use it in the ruby code) with which I can check whether we are in a multi ractor mode and AFAIK this is the only method which works differently even if not used in a ractor.

This makes fixing these specs kinda funky, as I have a custom guard to check the ractor mode and then run appropriate specs in it. But, the only guard I can create, is trying this method and if it throws running the multi-ractor spec. But, that defeats the purpose of the spec since that is kinda what we want to test.

Any suggestions on how I could approach this?

@dushyantss dushyantss force-pushed the ractors-access-module-instance-variables-specs branch from f0bc41c to 78bdf8f Compare June 1, 2023 09:47
@eregon
Copy link
Member

eregon commented Jun 1, 2023

Right. I think we should run the new specs in a subprocess then (via ruby_exe), which is how CRuby tests deal with this, no Ractor is created in the main test process. It is unfortunate.

In general I'm hesitant whether we should add specs for Ractor given it's still quite unstable in CRuby and there seems to be no much point so far for other Rubies to support it (Threads are more general and faster).

@dushyantss dushyantss force-pushed the ractors-access-module-instance-variables-specs branch from 78bdf8f to 13ef74a Compare June 1, 2023 19:59
@dushyantss
Copy link
Contributor Author

Thanks a lot for letting me know about ruby_exe, really useful for this particular set of specs.

I get your hesitation regarding ractors, but I think we should still have the specs here as ractors are a part of Ruby(however unstable and unused they may be for now 😅).

@dushyantss dushyantss marked this pull request as ready for review June 1, 2023 20:44
@dushyantss
Copy link
Contributor Author

Hey @eregon, are we planning to merge this one?

@eregon
Copy link
Member

eregon commented Feb 6, 2024

I'm thinking to close it actually, sorry.

First of all, having Ractor specs in language or core or library feels wrong.
I don't think it makes sense to count as failures on other implementations than CRuby, when even CRuby doesn't work well with Ractors, e.g. doesn't even dare to test Ractor in the same process as other tests.
It's a feature which is still pretty much broken in CRuby, incompatible with Ruby code in general, unfinished, etc.
It could be under optional/ractor or so I guess, but I don't see the value.
I think Ractor tests for now should remain in CRuby tests since it is very much an experimental CRuby-specific feature (not unlike RubyVM).

@eregon eregon closed this Feb 6, 2024
@dushyantss
Copy link
Contributor Author

Oh that's fine. Thanks for the reasoning.

@dushyantss
Copy link
Contributor Author

Could you also update the related issue to avoid others working on it?

@eregon
Copy link
Member

eregon commented Feb 6, 2024

Yes, I did that already and the same for 3.0, 3.1 and 3.2 issues.

@dushyantss
Copy link
Contributor Author

Awesome, thanks

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

Successfully merging this pull request may close these issues.

2 participants