Merge 1.1 into master (redux)

[chadlwilson]

• Alternative to #253 after getting tests to pass on master
• Fixes #259

Todo

• [ ] Get tests to pass, there are some logger things to figure out, probably screwed up in the merge

[kares]

:100: Chad, this is looking great :green_heart: indeed, it's been a while since 1.1 was merged to master (mostly due always being in a rush when working on yet another 1.1.x minor release). I was hoping to do some merging and cleanup work on master before 1.2.0 but than the week only has 7 days :sweat:. think we degraded the "production" quality expectations with the 1.2.x releases, at least for now.

anyways, I am very glad to see the work you did here. maybe some of it is redundant but there are a lot of important fixes that are otherwise missing from master, thus :+1: from me...

[chadlwilson]

Thanks @kares, nice to see you!

I think the main thing I could use some input on was the intent of the earlier changes to the way the loggers work across the branches, and whether they are still relevant these days. I wasn't really sure what the main philosophy was for 1.2 so was really just trying to "infer", which is probably a bit tough for me, as not a Rack expert.

For example, in https://github.com/jruby/jruby-rack/pull/262/commits/6abde22131da858660d97515546bba51436c5a0a I made the tests pass, but I'm not sure if I am merging something which should only apply to 1.1 and has no role in 1.2 and why I needed to do this. 😅

Given the divergence of the branches, there is a risk of (accidentally) reinstating some historical complexity from 1.1 that was intended to be removed/ignored on 1.2.

Other than that, my intention before un-drafting the PR was

• to actually sanity test it with at least my "real" application
• to play around with the examples and appraisals stuff (which I suspect do not work/pass anymore) to understand what confidence they give us and whether it's important to extend that to newer rails versions etc.

I note that the old travis setup seemed to run the appraisals, so at some point they passed, but right now they are not included in the GHA CI which makes me a little nervous. Even if many of those rails versions do not now need to be supported/appraised, it'd probably be good to run at least Rails 6.1 I imagine, while noting that at least in some use cases (#244) Rails 7.0 doesn't currently work right.

But ya, I also got a bit busy in personal life. Will have some more time/space back soon.

[kares]

That looks okay, in terms of the intent of the Logger implementation defaults changing in 1.2.0.

Unfortunately, I am not fully up to speed and do not have an app to test it, but seems the default is not setup to be 100% usable (at least with recent Rails), these days: https://github.com/jruby/jruby-rack/pull/260#issuecomment-2275439819

Not sure what is really missing but indeed the idea was that since we JRuby-Rack supports redirecting logging to a Java backend to fully support that, with plain old Ruby Logger that is a bit of a stretch thus the custom impl class...

[enebo]

@kares @headius so now that we got approval for this what is needed now? Land and release? Should we have some smoketest set up? I feel like this project has languished due to lack of knowing if it is safe to release.

A question I would like answered is how do we QA this? At this point even if we don't answer this I feel like we should release just to get feedback on what else is still broken (forward progress even if wrong probably is right for now). That said, I feel like the lack of confidence is paralyzing our ability to move forward.

[chadlwilson]

Interested in your questions also, but give me a week or so to sanity check this? I'm not blocked here, just been very busy moving house so haven't had time to make progress 🙏

I'll have a go at the appraisals but since they are not running on master right now either this doesn't make things "worse" for 1.2 so probably can be decoupled 😅

[kares]

Should we have some smoketest set up? I feel like this project has languished due to lack of knowing if it is safe to release.

I usually had a dummy sample Rails (as well as Sinatra) app that I would deploy to Tomcat as a .war and see how it's doing. Unless of course there were fixes which targeted other servers than I would deploy to multiple ones (e.g. if there was a Jetty related bug report) as needed...

[enebo]

I usually had a dummy sample Rails (as well as Sinatra) app that I would deploy to Tomcat as a .war and see how it's doing. Unless of course there were fixes which targeted other servers than I would deploy to multiple ones (e.g. if there was a Jetty related bug report) as needed...

@kares I don't doubt there could be a lot of deploys to test all the things supported but even a golden path deploy I think would help improve confidence. At one point I think @mkristian had a bunch of CI tests on jruby/jruby doing some tests with web containers.

[kares]

:+1: definitely, I recall those test from the main jruby repo. although they were a little opaque to debug, the benefits are clear for a project such as jruby-rack. believe it might be easier these days with something like test-containers (Docker), time is the only enemy :wind_chime:

[mkristian]

guees they disappeared because they were hard to debug but quite helpful to get JRuby work and ensure to work in different servlet contexts - classloader it was if I remember right

[headius]

I finally dug back to this in my inbox and could help land it... what needs to be done at this point?

[chadlwilson]

I finally dug back to this in my inbox and could help land it... what needs to be done at this point?

@headius At the moment I think it's on me to sanity check this in a real environment, plus look at the appraisals.

I did an initial sanity test with GoCD and #259 is definitely fixed. However right now I am having a weird problem with expected session cookies not being set by browser on some Rack-managed routes leading to duplicate Jetty session creation on some routes (which then causes Jetty failures) which I am trying to solve. I need to verify it is related to changes made on 1.2 branch or something else that's gone wroing in my env (perhaps incorrect Set-Cookie from server).

[headius]

Ok give me a shout if there's any way I can help!

[chadlwilson]

The problem above with my sanity test is unrelated to this change - there is some Firefox Total Cookie Protection change causing an issue with a same site iframe on http://localhost that doesn't happen in a real environment. Sigh :)

All seems to be working fine with jruby-rack so far with GoCD locally.

Will move on to see what I can do with the appraisals.

[headius]

Ok a month later, where do we stand? @kares @jlahtinen @chadlwilson I'd like to figure out the path forward.

[chadlwilson]

I got the appraisals to run but couldn't get them to pass for any Rails version that works with current JRuby versions. It failed with a similar error to https://github.com/jruby/jruby-rack/issues/244 (however rn my real world usage I only have that problem with Rails 7, not 5.2 or 6.x for whatever reason).

I dont know how strongly we feel about them but it's probably beyond my knowledge to fix. I could include the GitHub Actions to run them (as a separate workflow for now?) in this PR or a separate one but I'm not sure if its blocking or not.

[jlahtinen]

I try to find time to test our apps with this version in next 6 hours and report back.

Not sure if you need any other input from my side.

[jlahtinen]

diff --git a/src/spec/ruby/rack/servlet/response_capture_spec.rb b/src/spec/ruby/rack/servlet/response_capture_spec.rb
index 8ef3e38..b106b9b 100644
--- a/src/spec/ruby/rack/servlet/response_capture_spec.rb
+++ b/src/spec/ruby/rack/servlet/response_capture_spec.rb
@@ -103,11 +103,4 @@ describe org.jruby.rack.servlet.ResponseCapture do
     expect( response_capture.isHandled ).to be true
     expect( response_capture.getStatus ).to eql 200
   end
-
-  private
-
-  def servlet_30?
-    Java::JavaClass.for_name('javax.servlet.AsyncContext') rescue nil
-  end
-
 end
diff --git a/src/spec/ruby/spec_helper.rb b/src/spec/ruby/spec_helper.rb
index e586731..8906a9b 100644
--- a/src/spec/ruby/spec_helper.rb
+++ b/src/spec/ruby/spec_helper.rb
@@ -61,7 +61,7 @@ module SharedHelpers
 
   def servlet_30?
     return @@servlet_30 unless @@servlet_30.nil?
-    @@servlet_30 = !! ( Java::JavaClass.for_name('javax.servlet.AsyncContext') rescue nil )
+    @@servlet_30 = !! ( Java::javax.servlet.AsyncContext rescue nil )
   end
   private :servlet_30?

With these changes rspec specs passed and rake gem command generates a .gem file while using java 11.

[headius]

@jlahtinen Thanks for trying this out! Does the resulting gem also work as you expect?

@chadlwilson Seems like we could merge in @jlahtinen's patch and get this merged?

[chadlwilson]

Yes, those changes do seem to help getting the specs to pass with bundle exec rake specs on both Java 8 and Java 11 - as well as via mvn test which was working earlier - thanks! Not sure I understand why, but I'll not ask too many questions :D

I've made this change, and made some subsequent tweaks to get the CI running on all LTS Java versions as well as some minor dependency bumps. Strangely Java 17 is failing a single test on GHA where it passes locally though. Hmm.

[jlahtinen]

@headius I wasn't able to fully test resulting gem yet. I start that work now once again.

[jlahtinen]

about https://github.com/jruby/jruby-rack/actions/runs/12028436967/job/33531563307?pr=262

I saw same problem happening sometimes and sometimes not ... I didn't try to understand what could be the reason but maybe I (we) should ...

[chadlwilson]

I can always drop the tests for Java 17 and/or 21 if they are mysteriously unstable. Only added them in as were working locally and I didn't think it'd be an issue.

I can sometimes get a failure locally but haven't figured out a pattern yet.

[jlahtinen]

I am quite sure (not 100%) that I used Java 11.

And I have to continue testing this with our apps later. Now about 20h pause.

[chadlwilson]

Yes, the existing tests are stable under Java 8 and 11. I use jruby-rack in production with both Java 17 and Java 21 without issue so it's likely an issue specific to the tests - perhaps the mocking.

(edit: fixed now. There are race conditions that seem implicit to the way the tests are run with pooled applications and multiple runtimes that means that sometimes the code seems to be "too fast" and thus fails on later Java versions, probably due to subtle JVM changes.)

[chadlwilson]

From my perspective, this is ready to go now (or as good as it's productive to get for now).

I'll create a separate draft PR for the appraisals (basically that runs tests alongside Rails versions as the old Travis build did) and see if anyone can help me work on it/fix them :-)

[headius]

@chadlwilson This is great, thank you for your help! I think it's probably fine to merge now and we'll deal with the appraisals separately.

@kares What do you think? Shall we go ahead and merge? I know folks have been waiting on another release with recent patches and 1.1 stuff merged forward.

[jlahtinen]

I was not able to deploy 2 rails app to same tomcat9 with java 11 with this version of jruby-rack

Caused by: java.lang.LinkageError: loader constraint violation: when resolving method 'int org.jruby.RubyString.cat19(org.jruby.util.ByteList, int)' the class loader 'bootstrap' of the current class, java/lang/Object, and the class loader org.apache.catalina.loader.ParallelWebappClassLoader @53811376 for the method's defining class, org/jruby/RubyString, have different Class objects for the type org/jruby/util/ByteList used in the signature (java.lang.Object is in module java.base of loader 'bootstrap'; org.jruby.RubyString is in unnamed module of loader org.apache.catalina.loader.ParallelWebappClassLoader @53811376, parent loader java.net.URLClassLoader @22d8cfe0) at java.base/java.lang.invoke.MethodHandleNatives.resolve(Native Method) at java.base/java.lang.invoke.MemberName$Factory.resolve(MemberName.java:1070) at java.base/java.lang.invoke.MemberName$Factory.resolveOrFail(MemberName.java:1098) ... 55 more

We are currently running version https://github.com/jruby/jruby-rack/compare/master...jlahtinen:jruby-rack:kvp5 that allows to deploy more than 1 rails app to same tomcat9

[chadlwilson]

@jlahtinen While its hard to tell for sure, I'm not aware of anything changed in these changes from 1.1.x over the years that would materially change whether your hack in https://github.com/jruby/jruby-rack/commit/fdbcf6a0fe1d8c957bad7cb545ad2850b961d41e would work before/after these changes?

That error looks like there are perhaps two different underlying JRuby versions being loaded somehow, given it is complaining about org/jruby/RubyString between the bootstrap Tomcat classloader and the ParallelWebApp classloader - both Tomcat classloaders - and given that jruby-rack defines JRuby as provided scope, which requires you to supply an appropriate JRuby version in the right place yourself?

...unless it's something to do with the loggers.... it's been quite a while since I tangled with Tomcat classloaders.

[jlahtinen]

Maybe that error ok to have after merging this.

And lets resolve this multiple rails apps later?

[headius]

LinkageError

@jlahtinen I concur with @chadlwilson... that error seems like you have two copies of JRuby loading. Perhaps there's a JRuby jar in the bootstrap jars for Tomcat? For multi-app deployments, there should really only be the copies in each app and not a global one.

That method (RubyString.cat19) exists but is deprecated in 9.4 (replacement is catWithCodeRange added in 9.4.7.0). It will be removed in JRuby 11 (or whatever big release comes after 10.0). We should update jruby-rack to use the replacement method at some point, but it has been around for a long time.