JRuby support: UNIXSocket#recv_io/send_io workaround, pooled ...

[mrbrdo]

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.

[rails-bot]

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.

[mrbrdo]

So what do you guys think?

[headius]

We (JRuby folks) would really like to see this land, since it could help our users mitigate slow startup times.

[mrbrdo]

I am also working on UNIXSocket#recv_io/send_io support for JRuby, if that works then we don't need any of the io_helpers.rb stuff.

[rafaelfranca]

I like the direction of that is going but what do you think about having a module called ApplicaionImpl we have two different strategy modules that are included conditionally?

Instead of include ApplicationImpl we would have include Application::ForkStrategy

[grosser]

I like that too .. feels more rubyish ...

On Mon, Nov 23, 2015 at 5:20 PM, Rafael França [email protected]
wrote:

I like the direction of that is going but what do you think about having a
module called ApplicaionImpl we have two different strategy modules that
are included conditionally?

Instead of include ApplicationImpl we would have include
Application::ForkStrategy

—
Reply to this email directly or view it on GitHub
#449 (comment).

[mrbrdo]

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 screen (should be possible because of improvements in JRuby 9k). If that works it will greatly reduce the amount of necessary changes. It would only work with newer versions of JRuby 9k, though. But I think that's fine.

[mrbrdo]

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:

• rb-readline has to be used (e.g. in the Gemfile of the rails app), as the jline implementation that JRuby normally provides is too buggy and arrow keys don't work properly when IO is redirected. It will still work if rb-readline is not used, but readline will be buggy. rb-readline works on JRuby since the 9k release.
• when running something that uses readline (rails c) on a different terminal as where spring server was ran, readline doesn't work properly (arrow keys). we can probably find a solution for this at some later point, perhaps the difference is that I don't use PTY as spring does in the fork variant (JRuby doesn't support PTY right now)

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.

[headius]

JRuby support for UNIXSocket#recv_io/send_io/for_fd will be in 9.0.5.0. Thanks for your help, @mrbrdo :-)

[mrbrdo]

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 script or screen but had issues that I wasn't able to get to the bottom of). Once JRuby gets PTY at some point we can fix that too.

Connect a PTY when preloading in background
Readline + libedit gets messed up if it loads when not connected to a
terminal. This resolves that problem.

[mrbrdo]

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.

[mrbrdo]

ping @rafaelfranca

[mrbrdo]

@rafaelfranca hate to be a bother, but please take a look. would be sad for all the work to have been for nothing

[rafaelfranca]

This looks good to me. Could you rebase your branch? I'll merge after it.

[mrbrdo]

Will do. Is there any change needed due to the binstub change? mrbrdo@f591a30#diff-caa1bb5debf6e7c579b50374a10fae92L9

[rafaelfranca]

Maybe we should check if it is a windows machine there.

[jonleighton]

I think you could use RUBY_ENGINE here?

[jonleighton]

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?

[jonleighton]

https://github.com/jonleighton/spring-watcher-listen provides an idea of how this might be achieved as a separate project

[mrbrdo]

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!

[mrbrdo]

I did a rebase to current master and fixed what was pointed out above. I will check the tests and squash later.

[jonleighton]

OK, I think there are two related questions here:

1. Who will maintain JRuby / pool strategy support
2. Is it best to build JRuby / pool strategy support directly into Spring, or implement it as a plugin

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 Process#fork and making the implementation unapologetically UNIXy. (I similarly have no interest in supporting Windows.) Don't get me wrong, I'm all for it as a general idea, but it doesn't personally affect or motivate me.

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.

[mrbrdo]

@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
I think it's fairly obvious which changes were needed to enable this. I would really appreciate that. I did put quite a bit of effort into it so it would be a shame for it to go to waste.

[jonleighton]

Sure, I'll find some time to have a look through and give some ideas. I'm happy to work with you on this.

[jonleighton]

Ok here's my general idea:

1. Extract abstract classes for Spring::Application and Spring::ApplicationManager as you have already done. We can keep these APIs stable.

2. Implement a Spring.application_manager class accessor, and have spring-pool assign something like Spring.application_manager = Spring::Pool::ApplicationManager when it loads up (this is pretty similar to how spring-watcher-listen hooks into Spring)

3. With your current changeset, in lib/spring/application/boot.rb there are two points of divergence: the SPRING_SOCKET env variable and the instantiation of a Spring::Application::PoolStrategy rather than a ForkStrategy. I think the boot file is small enough that it would make sense to have spring-pool call into its own boot file rather than the stock Spring one. There is still some code we probably want to share in there so maybe it can be extracted into a class method. So the normal Spring boot file would become:

   Spring::Application::Forked.boot(UNIXSocket.for_fd(3))

   And your spring-pool boot file would be:

   Spring::Application::Pooled.boot(UNIXSocket.open(ENV.delete("SPRING_SOCKET")))

   The actual boot method would be defined on the abstract class and would do all the setup stuff that the current lib/spring/application/boot.rb does.

4. For the binstub, I thought about creating an extension point but I think it'll get messy. I think it would be better if we detect the lack of a Process.fork inside the Spring::Application::Forked, and just print an error message suggesting to the user to install spring-pool. (Installing spring-pool would ensure we never get to the point where Process.fork is needed.)

5. What's the story with the changes to IGNORE_SIGNALS and FORWARDED_SIGNALS? I haven't understood what that's about.

How does that sound? I think that covers it, but let me know if I've missed something.

[mrbrdo]

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 IGNORE_SIGNALS, it is not possible in JRuby to trap QUIT and USR1 because they are used by the JVM. If you try to trap it you get a warning, which is a bit too annoying in this case.

[MartinKoerner]

Is there any chance we see this merged some day? Or has either JRuby or spring moved too much in the past 2 years?