Repository navigation
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. |
|
@aminmansuri Clarify for me what's left to do here... with the EBADF fix from #9723 do we still need this change? |
|
Yes, still needed. They fix different things. #9723 fixed close after a thread is killed. This PR is about a server that is never killed: shutdown closes the socket while the thread is still waiting in accept. That raises IOError, on CRuby too, and the thread's rescue doesn't catch it, so the test fails. The fix does what CRuby does here: kill the thread first, then close. |
|
looking at the code should I put a To prevent a weird hang? If no timeout it could, under some very rare circumstances block forever. |
|
Ok, then we're back to my original problem with this change: why do we need to do it this way and CRuby just does def shutdown
@thread.kill
@server.close
endThese tests are synced from CRuby (which syncs with ruby/openuri, see https://github.com/ruby/open-uri/blob/master/test/open-uri/utils.rb#L29-L32) so any changes we make in our copy can easily get wiped out by future updates. |
|
@aminmansuri If you're still out there I'm still interested in resolving this. The ideal situation is to be able to use CRuby's test without modifications, but only if the setup+teardown it includes is expected to work properly on JRuby. If there's a problem with the test, we need to upstream that along with our local changes, so those changes don't get wiped out. |
|
I'm traveling right now so I can't get to it for a while. |
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.