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

MiniSSL.java - set serialVersionUID, fix RaiseException deprecation #3270

Merged
merged 2 commits into from
Oct 30, 2023

Conversation

MSP-Greg
Copy link
Member

Description

While working on the test framework, there have been consistent intermittent failures with JRuby, but never with the job that did not use SSL. The failures were much more consistent in CI, but much less common locally. But, I've got a lot of RAM.

The failures showed that all tests passed, but the test process freezes, and the CI step times out.

For quite a while, the following has been logged in the JRuby compile step. There are also case fall-thru warnings, but the case statements have integer switches, so multiple matches cannot happen:

ext/puma_http11/org/jruby/puma/MiniSSL.java:503: warning: [deprecation] RaiseException(Ruby,RubyClass,String,boolean) in RaiseException has been deprecated
    RaiseException ex = new RaiseException(runtime, errorClass, message, true);
                        ^
ext/puma_http11/org/jruby/puma/MiniSSL.java:49: warning: [serial] serializable class MiniSSL has no definition of serialVersionUID
public class MiniSSL extends RubyObject { // MiniSSL::Engine

Not being a java type, I googled and also looked at nio4r. In nio4r, serialVersionUID is set to what appears to be random long values. So, I added a similar statement to our java code, and the tests are completing. Or, no more frozen test processes.

@kares - sorry for the ping. Does this look okay to you? Is the RaiseException warning easy to fix? TIA...

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added (or updated) appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.

@kares
Copy link
Contributor

kares commented Oct 29, 2023

Hey, the serialVersionUID warning is a generic Java warning, nothing really to be concerned about.

The JRuby RaiseException deprecation can be fixed (since the gem requires Ruby >= 2.4 means JRuby >= 9.2):

diff --git ext/puma_http11/org/jruby/puma/MiniSSL.java ext/puma_http11/org/jruby/puma/MiniSSL.java
index 1b9ab433..87e9e27e 100644
--- ext/puma_http11/org/jruby/puma/MiniSSL.java
+++ ext/puma_http11/org/jruby/puma/MiniSSL.java
@@ -500,7 +500,7 @@ public class MiniSSL extends RubyObject { // MiniSSL::Engine
   }
 
   private static RaiseException newError(Ruby runtime, RubyClass errorClass, String message, Throwable cause) {
-    RaiseException ex = new RaiseException(runtime, errorClass, message, true);
+    RaiseException ex = RaiseException.from(runtime, errorClass, message);
     ex.initCause(cause);
     return ex;
   }

... but unfortunately I do not think the patch will have any effect for what you mentioned:

The failures were much more consistent in CI, but much less common locally. But, I've got a lot of RAM.

The failures showed that all tests passed, but the test process freezes, and the CI step times out.

@MSP-Greg
Copy link
Member Author

@kares

Thank you for the response and the patch. I've got JRuby running on WSL2/Ubuntu 20.04 & 22.04, and also macOS. If a CI run succeeds but freezes, I can repo locally using the seed from the CI run. The only process running is the test process.

Any suggestions as to how to find the issue? I have pretty much zero knowledge re java...

@MSP-Greg MSP-Greg force-pushed the 00-MiniSSL.java-serialVersionUID branch from 5729d03 to d125925 Compare October 29, 2023 19:13
@MSP-Greg MSP-Greg changed the title Update MiniSSL.java - set serialVersionUID MiniSSL.java - set serialVersionUID, fix RaiseException deprecation Oct 30, 2023
@MSP-Greg MSP-Greg merged commit c2fe0ed into puma:master Oct 30, 2023
60 of 64 checks passed
@MSP-Greg MSP-Greg deleted the 00-MiniSSL.java-serialVersionUID branch October 30, 2023 02:10
@kares
Copy link
Contributor

kares commented Oct 30, 2023

if you can reproduce than take a thread dump (also possible on CI but a bit involves a bit more clever code to be added) either using UI tools such as VisualVM or the command line using jstack (binary comes with the JDK).

@MSP-Greg
Copy link
Member Author

Thanks. I'll see if I can get that to work. One thing I haven't mentioned, we run a JRuby job without SSL, and I've never seen that freeze...

@kares
Copy link
Contributor

kares commented Oct 30, 2023

One thing I haven't mentioned, we run a JRuby job without SSL, and I've never seen that freeze...

maybe some kind of buffering issue, would mean we should see smt of MiniSSL.doOp near the top of the trace...

@MSP-Greg
Copy link
Member Author

MSP-Greg commented Oct 30, 2023

Thanks. jstack works, very helpful. Thank you.

Many seem to indicate a server isn't shutting down properly, as there are a lot of ThreadPool threads. I'm working on it today, more tonite...

BTW, the same test code is not interfering with the MRI CI jobs. Not sure about that, hopefully more later.

@MSP-Greg
Copy link
Member Author

MSP-Greg commented Nov 1, 2023

@kares

It seems that running the (new) test suite with JRuby intermittently has hung Reactor & ThreadPool threads. Same thing may exist with the current test suite, haven't tested it. Kind of frustrating, as MRI Rubys don't have the problem.

Regardless, started with 150 or so threads, now it's down to a handful...

@kares
Copy link
Contributor

kares commented Nov 2, 2023

It seems that running the (new) test suite with JRuby intermittently has hung Reactor & ThreadPool threads.

what do you mean by "(new) test suite"?
JRuby support for Reactor is pretty much non-existent at this point, if I am not mistaken.

maybe, let's open a ticket with all the details you have (e.g. jstack details) about what is hanging...

@MSP-Greg
Copy link
Member Author

MSP-Greg commented Nov 2, 2023

what do you mean by "(new) test suite"?

I'm working on an update to the test suite used by Puma. Puma uses NIO/nio4r for its 'reactor'. I'll post a thread dump in a new issue.

One thing I haven't tried is simply spinning up a Puma server and seeing if threads are cleaned up.

So, I don't know if it's due to Puma, or due to the test quite spinning up a lot of servers in a short time. Some of servers are 'in-process' servers (Puma::Server.new.run), some are spawned.

@kares
Copy link
Contributor

kares commented Nov 2, 2023

on the JRuby side do not worry too much about a "lot of threads" unless you can confirm they're executing some Ruby code that should have been done by now...

and yes, indeed things could be tricky, MRI due GIL does not have true concurrency so you might have code that works there but breaks under JRuby...

nateberkopec pushed a commit that referenced this pull request Jan 2, 2024
…3270)

* MiniSSL.java - fix compile warning - serialVersionUID

* MiniSSL.java - fix 'warning: [deprecation] RaiseException'

Co-authored-by: Karol Bucek <kares@users.noreply.github.com>
nateberkopec pushed a commit that referenced this pull request Jan 2, 2024
…3270)

* MiniSSL.java - fix compile warning - serialVersionUID

* MiniSSL.java - fix 'warning: [deprecation] RaiseException'

Co-authored-by: Karol Bucek <kares@users.noreply.github.com>
nateberkopec pushed a commit that referenced this pull request Jan 2, 2024
…3270)

* MiniSSL.java - fix compile warning - serialVersionUID

* MiniSSL.java - fix 'warning: [deprecation] RaiseException'

Co-authored-by: Karol Bucek <kares@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants