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

Merge 1.1 into master (redux - with all specs passing + bumped dependency minor versions) #262

Merged
merged 94 commits into from
Dec 2, 2024

Conversation

chadlwilson
Copy link
Contributor

@chadlwilson chadlwilson commented Jul 18, 2024

kares and others added 30 commits April 27, 2015 21:13
... (this is relevant since Bundler >= 1.10.1)
@chadlwilson chadlwilson marked this pull request as ready for review November 26, 2024 16:30
@chadlwilson chadlwilson changed the title Merge 1.1 into master (redux) Merge 1.1 into master (redux - with all specs passing + bumped dependency minor versions) Nov 26, 2024
@chadlwilson
Copy link
Contributor Author

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
Copy link
Member

headius commented Nov 26, 2024

@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
Copy link
Contributor

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 master...jlahtinen:jruby-rack:kvp5 that allows to deploy more than 1 rails app to same tomcat9

@chadlwilson
Copy link
Contributor Author

@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 fdbcf6a 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
Copy link
Contributor

Maybe that error ok to have after merging this.

And lets resolve this multiple rails apps later?

@headius
Copy link
Member

headius commented Nov 27, 2024

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.

@jlahtinen
Copy link
Contributor

I think we do not have a global one there.

Can you spot anything else that could be the reason?

[root@f69b8a4c8384 tomcat9]# pwd
/usr/share/tomcat9
[root@f69b8a4c8384 tomcat9]# find -name '*jruby*'
./foo_apps/ROOT/WEB-INF/gems/gems/concurrent-ruby-1.3.4/lib/concurrent-ruby/concurrent/synchronization/jruby_lockable_object.rb
./foo_apps/ROOT/WEB-INF/gems/gems/jruby-rack-1.2.2.pre1
./foo_apps/ROOT/WEB-INF/gems/gems/jruby-rack-1.2.2.pre1/lib/jruby
./foo_apps/ROOT/WEB-INF/gems/gems/jruby-rack-1.2.2.pre1/lib/jruby-rack-1.2.2-pre1.jar
./foo_apps/ROOT/WEB-INF/gems/gems/jruby-rack-1.2.2.pre1/lib/jruby-rack.rb
./foo_apps/ROOT/WEB-INF/gems/gems/nokogiri-1.16.7-java/lib/nokogiri/jruby
./foo_apps/ROOT/WEB-INF/gems/gems/racc-1.8.1-java/lib/java/racc/cparse-jruby.jar
./foo_apps/ROOT/WEB-INF/gems/specifications/jruby-rack-1.2.2.pre1.gemspec
./foo_apps/ROOT/WEB-INF/lib/jruby-core-9.4.8.0-complete.jar
./foo_apps/ROOT/WEB-INF/lib/jruby-rack-1.2.2-pre1.jar
./foo_apps/ROOT/WEB-INF/lib/jruby-stdlib-9.4.8.0.jar
./bar_apps/ROOT/WEB-INF/gems/gems/concurrent-ruby-1.3.4/lib/concurrent-ruby/concurrent/synchronization/jruby_lockable_object.rb
./bar_apps/ROOT/WEB-INF/gems/gems/jruby-rack-1.2.2.pre1
./bar_apps/ROOT/WEB-INF/gems/gems/jruby-rack-1.2.2.pre1/lib/jruby
./bar_apps/ROOT/WEB-INF/gems/gems/jruby-rack-1.2.2.pre1/lib/jruby-rack-1.2.2-pre1.jar
./bar_apps/ROOT/WEB-INF/gems/gems/jruby-rack-1.2.2.pre1/lib/jruby-rack.rb
./bar_apps/ROOT/WEB-INF/gems/gems/nokogiri-1.16.7-java/lib/nokogiri/jruby
./bar_apps/ROOT/WEB-INF/gems/gems/racc-1.8.1-java/lib/java/racc/cparse-jruby.jar
./bar_apps/ROOT/WEB-INF/gems/specifications/jruby-rack-1.2.2.pre1.gemspec
./bar_apps/ROOT/WEB-INF/lib/jruby-core-9.4.8.0-complete.jar
./bar_apps/ROOT/WEB-INF/lib/jruby-rack-1.2.2-pre1.jar
./bar_apps/ROOT/WEB-INF/lib/jruby-stdlib-9.4.8.0.jar

Another app works and another fails to

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 @5f4098bd 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 @5f4098bd, 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)

@kares
Copy link
Member

kares commented Nov 30, 2024

Chad has done a great job here, the actual production affecting changes are all from a stable 1.1 release, thus I am in favor of merging and cutting a new release.

The LinkageError is unclear, does not seem like changes in this PR would cause that and should potentially be a separate ticket.

@headius headius merged commit 2287eaa into jruby:master Dec 2, 2024
8 checks passed
@headius
Copy link
Member

headius commented Dec 2, 2024

Merged! @jlahtinen Let's get your issue in a separate report and figure it out quickly before a release.

@jlahtinen
Copy link
Contributor

@headius I created #268 didn't have too much time to think how to create a good report, sorry.

This was referenced Dec 5, 2024
@chadlwilson
Copy link
Contributor Author

@headius any chance of cutting a 1.2.x release off master, with these forward-ports in place?

I'd like to give an upgrade a go "in anger" for GoCD, if possible. Will make it a lot easier to see if I can solve #244 without other side effects.

@headius
Copy link
Member

headius commented Jan 21, 2025

@chadlwilson Yeah good idea. @jlahtinen @kares Anything extra we should include before release?

@chadlwilson
Copy link
Contributor Author

chadlwilson commented Jan 22, 2025

Actually as it turns out, I had a revelation as of yesterday and I think I have figured out #244 as well as gotten the appraisals to pass in a number of configurations. Working on it at chadlwilson#1

Actually I'm still rather confused about how any of 1.1.x or 1.2.2 works with rack 2.x and any modern Rails version to be honest, given the changes in rack sessions and store usage 😅

@jlahtinen
Copy link
Contributor

@chadlwilson Yeah good idea. @jlahtinen @kares Anything extra we should include before release?

I think only thing that affects our projects is how logging is different in puma. I started that work in #270

@kares
Copy link
Member

kares commented Jan 22, 2025

any modern Rails version to be honest, given the changes in rack sessions and store usage 😅

the servlet session store is an optional feature, so it's not really involved unless you configure it.
the tricky part is going to be supporting various versions on Rails/Rack with the servlet session store.

only thing that affects our projects is how logging is different in puma. I started that work in #270

because you use the Rails defaults in Puma, however jruby-rack has to support embedded deployments and thus the config.logger default (Logger.new("#{Rails.root}/log/#{Rails.env}.log")) is different. you can easily work-around that by configuring config.logger, in which case the jruby-rack logger would not get involved.

I would just ship Chad's changes and hopefully we'll have some time for PR reviews later.

@chadlwilson
Copy link
Contributor Author

I have all the appraisals working now on rails 5.0 through 7.2, with a change that makes things rack 2.x only (3.x doesn't work for other reasons)

Ill open a PR for that now, and we can discuss if we are fine dropping hacks for rack 1.x

@chadlwilson
Copy link
Contributor Author

(Rails 8 won't work until st least JRuby 10 as it drops 3.1 compatibility)

@chadlwilson
Copy link
Contributor Author

See #271

@jlahtinen
Copy link
Contributor

because you use the Rails defaults in Puma, however jruby-rack has to support embedded deployments and thus the config.logger default (Logger.new("#{Rails.root}/log/#{Rails.env}.log")) is different. you can easily work-around that by configuring config.logger, in which case the jruby-rack logger would not get involved.

Thanks for the answer. Any tips how to find what is the default logger in puma. So how to make sure logger stays the same rails default in puma and jruby-rack case?

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.

NoMethodError undefined method get_header re-emerged with JRuby Rack 1.2.2