Stop the open-uri test server racing its own socket close - #9718
aminmansuri wants to merge 1 commit into
Conversation
SimpleHTTPSServer closed its listening socket while its own thread was still blocked in accept, so the thread died on the teardown. Kill the thread and join it before closing, with the join guarded.
|
This is the one discovered during #9716 |
headius
left a comment
headius
left a comment
There was a problem hiding this comment.
This one also needs a deeper look.
This file is part of the MRI test suite copied from CRuby, so usually we don't like to make changes here. Obviously I did that already by removing the @thread.kill.
There's clearly something wrong with the sequence of events here when running on JRuby, so there's two possibilities I see:
- JRuby is doing something wrong internally that makes the kill+close sequence fail. In that case, JRuby should be fixed.
- The test is making assumptions about the kill+close sequence that it should not, in which case the test should be fixed.
You and I have attempted to fix the test, but I have always wanted more confidence that it's not a JRuby bug we're patching around.
The test fix might be the right way to go, but in that case we should also try to get this into open-uri.
I'd propose the following:
- Use the example case to determine whether JRuby is at fault. JRuby's full parallelism might make the test flaky (not our fault perhaps) or we might have a flaw in that event-passing that makes the test flaky.
- If we can agree JRuby is not at fault, we should raise an issue with open-uri that this simplistic server teardown needs a patch.
- Once accepted by open-uri we can patch it here and most likely it won't get wiped out by future updates.
|
A litte background here: This simple server logic is now sprinkled over various parts of the CRuby test suite after the WEBrick library got moved out of the default stdlib. Formerly, WEBrick was used to manage these test servers, and it has much more robust logic for starting up and shutting down servers. I've never been quite satisfied with the new tests. |
|
There is an example of the teardown you would want in test/mri/net/http/utils.rb — @thread.kill followed by @thread.join — and it still fails intermittently on JRuby. Two tests errored that way on #9718's own CI run, TestNetHTTPKeepAlive#test_keep_alive_get_auto_retry and TestNetHTTP_v1_2#test_get__implicit_start, both Errno::EBADF raised from IO#close at line 35, the socket&.close in the ensure inside the accept loop. The thread is killed, unwinds into that ensure, the close raises, and join re-raises it into the test. Java 21 and Java 26 failed; Java 25 passed on the same commit. That may be the better case to reason from, since the kill and join are already there, so nothing can be blamed on their absence. The teardown — @thread&.kill then @thread&.join: jruby/test/mri/net/http/utils.rb Lines 47 to 50 in 6718ce1 The accept loop, with the socket&.close that raises at L35: jruby/test/mri/net/http/utils.rb Lines 28 to 39 in 6718ce1 |
|
Reported JRuby issue as #9722. This one is separate: the open-uri teardown closes the server socket under a blocked accept with no kill, which raises on CRuby too — the helper's assumption, not an engine difference. |
2 participants
SimpleHTTPSServer#shutdownclosed the listening socket while its own threadwas blocked in
accept, so the thread died on anIOErrorits rescue doesnot cover and
assert_join_threadsfailed the test. Intermittent because thethread may instead be in the handshake, where the error is rescued.
CRuby kills the thread before closing for all three servers here; JRuby had
that commented out for this one. Kill is asynchronous, so the thread is also
joined, and the join is guarded — a bare one runs in the client thread's
ensure, masking the test's own failure, which is why one was removed fromthis method in April 2025.
net/http/utils.rbalready does kill-then-join.Refs #9276 — the same failure shape in this file's plain HTTP server.