-
Notifications
You must be signed in to change notification settings - Fork 341
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
JRuby support: UNIXSocket#recv_io/send_io workaround, pooled ... #449
base: main
Are you sure you want to change the base?
Conversation
Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @rafaelfranca (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
So what do you guys think? |
We (JRuby folks) would really like to see this land, since it could help our users mitigate slow startup times. |
I am also working on UNIXSocket#recv_io/send_io support for JRuby, if that works then we don't need any of the |
I like the direction of that is going but what do you think about having a module called Instead of |
I like that too .. feels more rubyish ... On Mon, Nov 23, 2015 at 5:20 PM, Rafael França notifications@github.com
|
Yes I agree, that sounds better. I'll take that into account. At the moment I almost got rid of the IO abstraction by implementing recv_io/send_io, but still need jnr/jnr-unixsocket#4 to be able to completely remove that. Then I will try to do IO redirection without using |
Ok so here is the new, much slimmer version. It will only work once JRuby gets support for UNIXSocket#recv_io/send_io/for_fd (in progress, we're almost there). I'm not sure if we need to do something extra for upgrading the binstubs (as is already done for some older version)? Also will need to update the README I guess. There are two caveats with this version, which the screen version didn't have:
But I think the readline issue is worth not having to use screen and weird hacks to make it look more like it's not a screen (preserve history). This new way works much more like spring works with fork. |
JRuby support for UNIXSocket#recv_io/send_io/for_fd will be in 9.0.5.0. Thanks for your help, @mrbrdo :-) |
514ab01
to
f591a30
Compare
Based on this commit message 6e0a6e8 I guess I was right - readline has problems when the terminal is not the same because we are not using PTY in the pool implementation. I tried a few workarounds but unsuccessfully (tried running through
|
Hey guys. The required functionality was just merged into JRuby master and it should be available on 9.0.5.0. So we can begin thinking about where to go with this. |
ping @rafaelfranca |
@rafaelfranca hate to be a bother, but please take a look. would be sad for all the work to have been for nothing |
This looks good to me. Could you rebase your branch? I'll merge after it. |
Will do. Is there any change needed due to the binstub change? mrbrdo@f591a30#diff-caa1bb5debf6e7c579b50374a10fae92L9 |
Maybe we should check if it is a windows machine there. |
"jruby" | ||
else | ||
"ruby" | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you could use RUBY_ENGINE
here?
Hi folks, I've had a look at this and made a few comments. I'm up for the general idea of creating a way for JRuby users to benefit from Spring. However, I'd like to consider making this a separate gem, rather than merging it into Spring core. That would allow @mrbrdo to take a lead on maintaining the spring-pool (or whatever) project, while we focus on the forking version here. It would also force us to create the right abstractions to allow the spring-pool project to operate, which will result in a cleaner implementation, I think. Rails itself could be modified to include the spring-pool gem in the default Gemfile when on JRuby. Thoughts? |
https://github.com/jonleighton/spring-watcher-listen provides an idea of how this might be achieved as a separate project |
Well I would actually prefer not to be burdened by maintaining this, and it's even harder to do that when it's outside of the core gem. Another argument would be that the pool strategy is more basic and works on more platforms, so it cool to have that option. But in any case, some of these changes would have to be incorporated into the core gem anyway, even if the pool strategy would be in a separate gem, because without certain refactoring it is almost impossible to patch stuff externally. I did my best with the refactoring. Thanks for the code review! |
I did a rebase to current master and fixed what was pointed out above. I will check the tests and squash later. |
OK, I think there are two related questions here:
I have concerns about just merging this patch without an explicit person who is championing JRuby support. It is a fairly big change, and I think it is almost inevitable that once we start advertising JRuby support the bug reports and PRs will start coming in and somebody will need to deal with them. As it is, I already struggle to find the time to deal with the existing PRs and crucial bug fixes which come in for Spring. Other people do help with that (thanks!) but I still feel that a lot of the more complicated changes which happen end up needing my involvement because I'm the original author of Spring and probably the person who has most knowledge about the various nuances of how it works and why certain things are done in certain ways. (Much as I've tried to make the code clear, and document my commits, there are still a lot of fiddly details...) I'm not a JRuby user and I have no personal interest in supporting JRuby. This was a driver in my original decision to go all-out with using In short, I think merging this as-is, with nobody stepping up as maintainer, is likely to increase the amount of work that I'm expected to do. Even if I just have to merge PRs, it's still quite possible that those PRs might break Spring in unexpected ways (perhaps for non-JRuby users too) and then somebody will have to pick up the pieces. We were in a similar position with the listen watcher a while back - it wasn't something I actually used but lots of people were commenting on a PR saying "why haven't you merged this yet!" and I felt like I was a bottleneck. Now that it's separate, those people who care about listen support can look after themselves. I suppose all this drives my desire to have a separate plugin implement the bulk of this rather than having the code in "Spring core". I think it provides a cleaner line of separation, and it will minimise the amount of code/PRs/bugs that I need to worry about. I fully understand that changes need to be made in "spring core" to provide the extension points to enable this and I'm perfectly fine for those changes to be made. But I'd like spring-pool to be a separate project with a separate maintainer who is personally motivated by its aims. |
@jonleighton fair enough. Do you think you could help out with breaking my changes apart to those that should be included in core? I am not quite sure how you'd want to approach certain things, such as mrbrdo@69fc4ab#diff-7d80bbcaf19996f457e297bea83fe4f5R6 |
Sure, I'll find some time to have a look through and give some ideas. I'm happy to work with you on this. |
Ok here's my general idea:
How does that sound? I think that covers it, but let me know if I've missed something. |
Thanks for looking into this. Sounds good, although I'm not yet quite sure how spring plugins work exactly (especially boot.rb, I thought that was a bit of magic I haven't looked that much into yet). But it seems like a plausible solution. Regarding |
Is there any chance we see this merged some day? Or has either JRuby or spring moved too much in the past 2 years? |
Hi guys.
I don't really expect this to be fully merged, although it might be feasible in some capacity. I am looking for some help, based on this PR, on how to create a spring plugin from this. With some refactoring in spring, as I have done here, it should be possible to pretty cleanly override what is needed in a plugin. As JRuby does not support recv_io/send_io, it would be crucial to use an UNIXSocket abstraction similar to what I've done.
I've put effort into changing the base spring code as little as possible, I really just refactored some parts into separate methods or Modules. I believe the extraction into Modules would not really be necessary as I could use Module#prepend in a plugin instead.
The real change is a replacement for ApplicationManager (with the same API), which holds a pool of preloaded apps, each running in
screen
. Instead of doing IO redirection, the client then exec's to connect to the screen. This is based on my older JRuby Rails preloader theine - https://github.com/mrbrdo/theine I did not find IO redirection to be feasible under JRuby, iirc mostly due to issues with readline.I have tested this with a JRuby 9k app, but I have not tested (for regression) on MRI yet. There is no intended change for MRI, except code refactoring.
Thanks for any suggestions or help.