Add JRuby and TruffleRuby to CI

_I'd like to add CI for JRuby and TruffleRuby, but the last time I tried I got a bunch of errors (admittedly, I didn't spend much time trying to figure them out).

Ideally, we could fix the errors without significant changes, and net-imap could be fully functional under those implementations.

If the changes needed for full compatibility are significant, maybe we can just mark some tests as pending. That will at least serve as a warning of what's incompatible, and prevent new changes from inadvertently widening that gap (see for example #452).

Fixed by

Other related issues

`net-imap` issues/PRs

♻️ Short-circuit frozen SequenceSet modifications #473 (JRuby FrozenError doesn't use #inspect)
✅ Update Regexp.linear_time? tests for non-CRuby #477
✅ Update ResponseReader, UIDFetchData, DeprecatedClientOptions tests #476
✅ Add timeouts to CI workflow #478
🐛 Fix loading of net/imap for JRuby/TruffleRuby #530
✅ Test overriding inherited ::Data methods #531
✅ Fix flaky tests with FakeServer#Connection#close mutex #520
Don't wait for lock before closing the socket in #disconnect

JRuby issues/PRs

Data fixes and improvements jruby/jruby#8829 - to fix Data subclasses
Splat with multi-line expression improperly expands jruby/jruby#8830 - fixed the response parser tests
Prism compiler fails with lonely operator in complex call sequence jruby/jruby#8832
JRuby isn't escaping strings correctly json#870
Struct inheritance issues: 002db9f.
All (or nearly all) of the SSL connection tests are failing. Hopefully this only requires small tweaks to net-imap test code.

TruffleRuby issues/PRs:

Misc

I either omitted or marked as pending everything that's currently failing.
TODO: diagnose deadlock on error (between get_tagged_response and receive_responses?)
Both JRuby and TruffleRuby still fail in various ways when inheriting from Data.
- I temporarily added in a crazy feature-check to our Data polyfill. Rather than just check RUBY_ENGINE and RUBY_ENGINE_VERSION and defined?(::Data) and ::Data.respond_to?(:define), it tries to use all of the features that were causing tests to fail. If anything looks off, it prints a warning and falls back to the polyfill. I'm not going to merge this feature check. But it's in the branch. 😉
- net-imap v0.6.0 will still be deleting DataLite.

Note: I rebased my previous work https://github.com/ruby/net-imap/tree/ci-alt-ruby-impls and tried a few more things. But TruffleRuby still has 9 failures, and JRuby isn't even successfully running the tests yet.

@nevans

It would be great indeed.
In general we're trying to add truffleruby to all default & bundled gems CI (truffleruby/truffleruby#2644) to avoid breakage and regressions like #452.

Regarding the failures, I think it's always better to run all tests except these 9 known failures, than to test nothing.
So I would suggest to add TruffleRuby in CI and omit these 9 failures for now.
It'd be great to have the output of the failures somewhere so we can look if there are some which are easy to fix.

@nevans Could you do that since you already have that branch?
If you would like some help (e.g. we make the PR based on your branch), @andrykonchin or I can help, let us know what you prefer.

@nevans

@nevans I'll have a look at failures a bit today.

The "crash" appears to be something missing in JRuby 10's Data impl. Interesting, since we pass all tests and specs related to Data currently.

Fix two behaviors of Data

The crashing was all due to some missing features of Data: subclass behavior and empty definitions (no fields). I've fixed those in jruby/jruby#8829.

The specs run now, but hang because the "fake server" implementation does not appear to work properly on JRuby. Most of those tests, the server terminates early. Some of them do not handle this well and end up hanging waiting forever for a server that has gone away.

I'll keep poking at it.

I'm making some progress but it was complicated by a deadlock in the net-imap tests. I've

There seems to be a bug in JRuby where the response name is not getting set right:

#<struct Net::IMAP::TaggedResponse tag="RUBY0001", name=" ", data=["OK", #<struct Net::IMAP::ResponseText code=nil, text="LOGOUT done">], raw_data="RUBY0001 OK LOGOUT done\r\n">

When this is handled by get_tagged_response, it falls off the end of the case and tries to force a disconnect:

net-imap/lib/net/imap.rb

Line 3457 in f76d433

disconnect

Disconnect tries to close down the socket and then wait for the receiver thread to exit. When the socket gets closed down, the receiver thread proceeds to try to call state_logout!:

net-imap/lib/net/imap.rb

Line 3350 in f76d433

state_logout!

Unfortunately both send_command (which calls get_tagged_response) and state_logout! call synchronize on the same object, which means the receiver thread gets stuck waiting for the lock that the main thread is still holding.

It does not affect the normal exit from get_tagged_response because it doesn't try to force a disconnect before leaving the synchronize. The receiver thread gets shut down normally elsewhere.

Obviously the bug in constructing the response needs to be fixed in JRuby, but this code shouldn't deadlock when the server's response is not in the expected valid responses.

The garbage responses were due to an apparent bug in JRuby's parser or compiler: jruby/jruby#8830

Getting closer to a full run, but something is still causing the fake server + test combination to start timing out. Is there some cross-test state that might be getting damaged, like a single test server or something?

FWIW I have to say this is one of the more unusual pieces of code I've run into, and it seems really fragile:

def response_tagged
  TaggedResponse.new(tag, *(SP!; resp_cond_state), @str)
end

This took me some time to unravel, because tag, SP!, and resp_cond_state are all calls, and at least the first two make state changes during the set-up of the arguments list.

My first attempt to tweak this was to move the entire SP!; resp_cond_state out, but that didn't work because one or both of them depend on state changes made by tag. Changing the order of these calls is non-negotiable, and I think it would be better if that were clear.

def response_tagged
  t = tag
  SP!
  TaggedResponse.new(t, *resp_cond_state, @str)
end

Running JRuby with the Prism parser works ok for the original code, but it still a bit of a worry.

@headius

@headius Thanks for looking through this. I hadn't seen your latest message and I was just about to post what looks like a bug in JRuby... which is in your last message. :)

Foo = Struct.new(:a, :b, :c, :d)
def ignored = :ignored
def tuple   = ["b", "c"]
Foo.new("a", *(ignored; tuple), "d")
# CRuby: => #<struct Foo a="a", b="b", c="c", d="d">
# JRuby: => #<struct Foo a="a", b=:ignored, c=["b", "c"], d="d">

I'll grant that it's not a normal idiom, but it doesn't seem particularly fragile to me... what other order could that code possibly execute in? It seems more like a misinterpretation of *(foo; bar). There shouldn't be any difference between fn(*(a; b)) and fn(*method_that_calls_a_then_b_and_returns_the_result_from_b), right? But if the Prism parser works, maybe we can just enable that for the JRuby tests? 🙂

Anyway, yes, not a normal idiom. I used that unusually terse form because that it's much easier (for my eyes at least) to line up with the ABNF than the exploded form. It's a style that may be harder to read at first, but in very specific contexts (like matching thousands of lines of ruby against thousands of lines of RFC ABNF) it can be become easier, at least over time, at least for my eyes. I've found several bugs by converting to that style.

Unfortunately both send_command (which calls get_tagged_response) and state_logout! call synchronize on the same object, which means the receiver thread gets stuck waiting for the lock that the main thread is still holding.

It does not affect the normal exit from get_tagged_response because it doesn't try to force a disconnect before leaving the synchronize. The receiver thread gets shut down normally elsewhere.

Obviously the bug in constructing the response needs to be fixed in JRuby, but this code shouldn't deadlock when the server's response is not in the expected valid responses.

That bit of code has been on my radar to refactor/rewrite for a while now. My biggest deployment of net-imap actually uses a completely different implementation for the receiver thread. This is good: now I have a concrete motivation and test-case for rewriting it. 😉

Oh, but the state change code is relatively new (#416), which means that that specific deadlock is new, too. I had wanted to handle those connection state changes inside the receiver thread (before signaling get_tagged_response), but that required bigger changes than I wanted to make at the time.

The code should work, don't get me wrong. We will have a fix in JRuby shortly.

The problem with that code, though, is that it is non-obvious that there's state changes happening behind the scenes. In general I strongly prefer that the argument list to a function not contain order-sensitive calls, as it does here. If order is important, make those calls separately before passing the results as arguments. It is very easy for a refactoring tool to break the code you have currently, because it will make the same assumption I did (that the order in which the arguments get evaluated is not important).

Speaking of interesting code... I switched to the Prism parser to work around the *(foo; bar) bug, since it works there, and then ran into a bug in our Prism-based compiler!

jruby/jruby#8832

This codebase is a treasure trove of exotic code forms!

FWIW since you are actively working on this library, I'm happy to continue collaborating on improvements! I may step back for a day or two due to some upcoming releases we are doing and to let the fixes for these two compiler issues land, but I very much want to get this library running 100% and fix whatever is needed along the way. As I see code that could be improved, I'll keep feeding that information back to you. Would you be interested in specific issues for things I find, like the deadlock and unusual code above?

it is non-obvious that there's state changes happening behind the scenes

That's context sensitive, IMO. In other codebases, I generally aim for Command Query Separation. If I'd written it from scratch, I probably would've preferred to use something more like parser combinators, or build a grammar definition and generate the parser. But in this class, nearly every method call potentially changes state. In that particular file, it's much more surprising when a method call won't change state behind the scenes! (Tangentially, if rbs added type information for which method calls are potentially mutating, I'd use it! Because without that information, naively relocating method calls is never a safe refactoring!)

This codebase is a treasure trove of exotic code forms!

😆 Some of the code goes back to ruby 1.6. But I guess the most "exotic" bits are probably more recent (and probably my fault). I use some styles in the response parser that I rarely use anywhere else. But one of the things both JRuby and TruffleRuby seem to struggle with is subclassing Struct and Data, and I've used subclasses of those quite often. I didn't think it was particularly exotic, and I'm honestly surprised my use-cases weren't covered by ruby's specs for those classes... I learned one of the subclassing tricks from the bugs.ruby-lang.org ticket that created Data!

As I see code that could be improved, I'll keep feeding that information back to you. Would you be interested in specific issues for things I find, like the deadlock and unusual code above?

Absolutely, yes please. 🙂 Even if I get defensive on certain style choices, please! And thanks!

@headius

@headius A question about the prism parser: I thought that adding .jrubyrc with parser.prism=true should enable it, but then I got "Failed to load prism: Using default parser provider". I tried gem installing it and adding it to the Gemfile, and those didn't appear to work?

At one point, I thought I'd had it working... but in retrospect, I had probably rbenv switched just beforehand and forgotten.

@nevans

@nevans You probably just installed the regular prism parser gem. We can use that, but as an external library for tools needing a parser. In order to install Prism for JRuby itself, gem install jruby-prism-parser. Then the option should work.

Navigation Menu

Repository navigation

Add JRuby and TruffleRuby to CI #454

Fixed by

Other related issues

`net-imap` issues/PRs

JRuby issues/PRs

TruffleRuby issues/PRs:

Misc

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Add JRuby and TruffleRuby to CI #454

Description

Fixed by

Other related issues

net-imap issues/PRs

JRuby issues/PRs

TruffleRuby issues/PRs:

Misc

Activity

nevans commented on Apr 27, 2025

eregon commented on May 5, 2025

headius commented on May 5, 2025

headius commented on May 5, 2025

headius commented on May 5, 2025

headius commented on May 5, 2025

headius commented on May 6, 2025

headius commented on May 6, 2025

nevans commented on May 6, 2025

nevans commented on May 6, 2025

nevans commented on May 6, 2025

nevans commented on May 6, 2025

headius commented on May 6, 2025

headius commented on May 6, 2025

nevans commented on May 6, 2025

nevans commented on May 6, 2025

nevans commented on May 6, 2025

headius commented on May 7, 2025

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

`net-imap` issues/PRs