Fixes to get WEBrick tests greener #6246
Conversation
|
The failure in webrick's test_cgi appears to be a bug in how we handle multibyte content passing through environment variables. The test tries to pass through a URI like "/webrick.cgi/%A4%DB%A4%B2/%A4%DB%A4%B2" using a CGI subprocess. The URI is parsed out and passed via an environment variable, which in JRuby ends up passing through a Java String. That String conversion appears to mangle this string, probably because it doesn't seem to be valid UTF-8. The code below is responsible, but I'm not sure how to get these env values to pass through to setenv without getting mangled. |
|
I don't believe we'll be able to fix the ENV issues without moving completely away from Java's own I will file a separate issue for this and likely expand the WEBrick test to also exclude the "java" platform. |
|
See #6248 for the ENV issue. |
|
The next failure, test_big_bodies in test_httpproxy, took some untangling. There's many layers to this test, but ultimately I found a WEBrick bug here: This is a pretty big assumption about how I will file a WEBrick bug (and PR if possible) to fix this. |
See jruby#6246 and ruby/webrick#43 for a discussion of this change.
|
The final CJK issue appears to be a bug in our It proceeds to run with that directory as the server's file base, and writes another file with the "あ" character in it. However when attempting to search for that file in the server's base, it uses We then fail to see this as a directory and fail the overall file search. |
|
Reduced case: The base path for the server appears to be ASCII-8BIT, which ends up breaking JRuby's logic for |
|
WEBrick does indeed appear to be forcing this path to be "binary", which leads to the |
In order to be able to expand paths that are marked as binary, we need to be able to treat those paths as raw bytes. This change attempts to decode those paths as ISO-8859-1 bytes, allowing the path expansion to ignore any multibyte characters rather than improperly decoding them. The final encoding is used to put the ISO-8859-1 bytes back into their 8-bit form and re-mark them as the eventually negotiated encoding. This fixes part of the WEBrick "cjk" test failure by allowing the binary-encoded multibyte path to be expanded without mangling the MBC character. This change is limited to when the incoming strings are explicitly marked as "binary" using the ASCII-8BIT encoding, since that case clearly has no encoding hint for us to use to get characters. There may be other cases, like CR_UNKNOWN or CR_BROKEN, that also deserve this treatment, but for now the change is limited to explicitly binary strings. See jruby#6171 for the general WEBRick bug and jruby#6246 for the PR that attempts to fix failures.
Our current logic for dealing with file existence is intertwined with a lot of JDK file APIs, so we don't have the option here of just using a raw-encoded string. This change attempts to use the system default encoding when the incoming path is specified as "binary" so that hopefully its characters will be decoded properly. This fixes cases where a properly-encoded multibyte path is marked as binary, as is the case in the WEBrick file-serving pipeline described in jruby#6246.
Our current logic for dealing with file existence is intertwined with a lot of JDK file APIs, so we don't have the option here of just using a raw-encoded string. This change attempts to use the system default encoding when the incoming path is specified as "binary" so that hopefully its characters will be decoded properly. This fixes cases where a properly-encoded multibyte path is marked as binary, as is the case in the WEBrick file-serving pipeline described in jruby#6246.
When using the reflective proxy generator, the crypt function will fail to cascade through various possible libraries, instead failing immediately on first call with the first library attempted. This breaks some tests on jruby#6171.
See jruby#6246 and ruby/webrick#43 for a discussion of this change.
UnresolvedSuper means we don't know the name we'll call, and must discover it from the frame. The super isn't "unknown", it's just determined "dynamically" at runtime.
b44d02b to
f130aaf
Compare
|
I've rebased this branch off master after merging in #6124 That PR represents a number of fixes to avoid mangling file paths that are marked as binary but which actually contain legitimate multibyte characters. There are two strategies taken:
With these additional changes the CJK failure is now passing. I will run some final checks but I believe I've done all I can on the JRuby side for the WEBrick test suite. |
|
With all these changes and the fix from ruby/webrick#45 I am now down to just a few failures. I include them below. The test_sni failure appears to happen consistently, and the other two are sporadic (only saw the concurrency issue once in dozens of runs). And some full traces below: |
|
The line of code in question for the concurrency error is adding two arrays that are being concurrently modified. From test/lib/webrick/unit.rb: def start_server(klass, config={}, log_tester=DefaultLogTester, &block)
log_ary = []
access_log_ary = []
>>> log = proc { "webrick log start:\n" + (log_ary+access_log_ary).join.gsub(/^/, " ").chomp + "\nwebrick log end" }
config = ({
:BindAddress => "127.0.0.1", :Port => 0,
:ServerType => Thread,
:Logger => WEBrick::Log.new(log_ary, WEBrick::BasicLog::WARN),
:AccessLog => [[access_log_ary, ""]]
}.update(config))
...The arrays here blew up just this one time, most likely due to the backing array or offset values getting changed out mid-addition. I will not attempt to fix this, but it's good justification for us to finally make concurrent array access robust enough to avoid locks. |
|
The (first) failure in I filed jruby/jruby-openssl#201 for this issue, but after some investigation I've determined there's no way to implement this callback on top of the Bouncy Castle RSA key pair generation. I believe this particular test will need to be skipped on JRuby. |
|
With the key pair generator callback assertion commented out, the remaining assertions also fail due to missing I believe that |
|
The final issue here, in First, I have seen this error: This is caused when the diff --git a/test/webrick/test_server.rb b/test/webrick/test_server.rb
index 8162a18..0d944a0 100644
--- a/test/webrick/test_server.rb
+++ b/test/webrick/test_server.rb
@@ -149,14 +149,15 @@ class TestWEBrickServer < Test::Unit::TestCase
:Port => 0,
:Logger => warn_flunk)
2.times {
- server_thread = Thread.start {
- server.start
- }
client_thread = Thread.start {
sleep 0.1 until server.status == :Running || !server_thread.status
server.stop
sleep 0.1 until server.status == :Stop || !server_thread.status
}
+ server_thread = Thread.start {
+ server.start
+ }
+
assert_join_threads([client_thread, server_thread])
}
endThe second failure I have seen is the "killed thread" failure mentioned above, which also appears to be happening during the start callback. I suspect this would either be fixed by the above, or by adding some confirmation that the threads in question have actually started. In short, it's another race on thread status. There are no further changes I can make in JRuby to make this test green. It fails sporadically due to races in the test itself. |
|
This PR is ready for review! |
This also addresses review comment from @kares to make this not be an instance inner class. There amy be a need for some code to be able to rescue this exception specifically, so I also made it public and made it a top-level class.
These are small fixes to address failing tests in WEBrick, as reported in #6171.