Bug 57530 - Reimplement TestAbstractHttp11Processor.testNon2xxResponseWithExpectation test using SimpleHttpClient instead of Java 6
Summary: Reimplement TestAbstractHttp11Processor.testNon2xxResponseWithExpectation tes...
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 7
Classification: Unclassified
Component: Examples (show other bugs)
Version: 7.0.57
Hardware: PC All
: P2 enhancement (vote)
Target Milestone: ---
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-02-03 13:31 UTC by Konstantin Kolinko
Modified: 2015-06-18 11:15 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Konstantin Kolinko 2015-02-03 13:31:24 UTC
As was noted when testing 7.0.59 release candidate, the testNon2xxResponseWithExpectation test in org.apache.coyote.http11.TestAbstractHttp11Processor fails when running with Java 6,

[[[
Testcase: testNon2xxResponseWithExpectation took 0,195 sec
	Caused an ERROR
null
java.lang.NullPointerException
	at org.apache.coyote.http11.TestAbstractHttp11Processor.doTestNon2xxResponseAndExpectation(TestAbstractHttp11Processor.java:646)
	at org.apache.coyote.http11.TestAbstractHttp11Processor.testNon2xxResponseWithExpectation(TestAbstractHttp11Processor.java:604)
]]]

Reference:
http://mail-archives.apache.org/mod_mbox/tomcat-dev/201502.mbox/%3CCABzHfVngj3_P1sn7fDGVk6Pys4nxzie9LQB%3DN5n6PYX%2BSAh7cQ%40mail.gmail.com%3E
http://mail-archives.apache.org/mod_mbox/tomcat-dev/201502.mbox/%3CCABzHfVnFn2VH+2kth2SZP-5CKPsi4oz6cb9q-wwsLw_2TKOMxQ@mail.gmail.com%3E


The test sets up a web application with a security constraint that
forbids access to an echo servlet. It sends a request with "Expect:
100-continue"  and expects a 403 response with "Connection: close"
header.

Running with a debugger, Java 6u45 does not send the "Expect:
100-continue" header, as the test expects.


I have not found a java bug report for this specific issue, but from those that I found I think that 
- handling of "Expect: 100-continue" is a late addition (since Java 7 only?) and
- it is more complicated than necessary for this simple test (HttpURLConnection handles the full request-response cycle, including 100 (Continue) response, timeouts etc).

I think that this case can be reimplemented using a more simple client implementation,

org.apache.catalina.startup.SimpleHttpClient

I also expect that Tomcat rejects the request immediately without sending a 100 (Continue) response. This detail can be tested by reimplementing the test with SimpleHttpClient.



(For reference:
http://bugs.java.com/view_bug.do?bug_id=6726695
JDK-6726695 : HttpURLConnection shoul support 'Expect: 100-contimue' headers for PUT

http://bugs.java.com/view_bug.do?bug_id=8012625
JDK-8012625 : Incorrect handling of HTTP/1.1 " Expect: 100-continue " in HttpURLConnection
)
Comment 1 Rainer Jung 2015-04-15 14:54:49 UTC
> http://bugs.java.com/view_bug.do?bug_id=8012625
> JDK-8012625 : Incorrect handling of HTTP/1.1 " Expect: 100-continue " in
> HttpURLConnection

That one is said to be fixed by today's release of 1.7.0_80:

http://www.oracle.com/technetwork/java/javase/2col/7u80-bugfixes-2494167.html

Regards,

Rainer
Comment 2 Christopher Schultz 2015-04-16 13:10:30 UTC
It looks like the test code is missing a call to HttpURLConnection.setFixedLengthStreamingMode() or HttpURLConnection.setChunkedStreamingMode(), which would have (?) made this work with older versions of the JRE.
Comment 3 Violeta Georgieva 2015-06-09 08:37:31 UTC
Hi,

What do you think if we add the code below.
Currently "Expect" header is restricted and because of this it is not added as request property.
If we set "sun.net.http.allowRestrictedHeaders" to "true" then it is added and the test passes.

Regards,
Violeta



Index: TestAbstractHttp11Processor.java
===================================================================
--- TestAbstractHttp11Processor.java	(revision 1684193)
+++ TestAbstractHttp11Processor.java	(working copy)
@@ -42,6 +42,7 @@
 import static org.junit.Assert.assertTrue;
 
 import org.junit.Assert;
+import org.junit.Before;
 import org.junit.Test;
 
 import org.apache.catalina.Context;
@@ -57,6 +58,13 @@
 
 public class TestAbstractHttp11Processor extends TomcatBaseTest {
 
+    @Before
+    @Override
+    public void setUp() throws Exception {
+        super.setUp();
+        System.setProperty("sun.net.http.allowRestrictedHeaders", "true");
+    }
+
     @Test
     public void testResponseWithErrorChunked() throws Exception {
         Tomcat tomcat = getTomcatInstance();
Comment 4 Konstantin Kolinko 2015-06-09 09:40:32 UTC
(In reply to Violeta Georgieva from comment #3)

Interesting.

This fix looks like having a too wide scope for a single failing test, but I am OK with it.

Setting the system property affects all the tests in the test class (as they are run in random order in the same JVM), so setting it in a @Before method looks fair. I do not expect any negative consequences from that property.


(In reply to Konstantin Kolinko from comment #0)
> 
> I think that this case can be reimplemented using a more simple client
> implementation,
> 
> org.apache.catalina.startup.SimpleHttpClient
> 

TestFormAuthenticator has an example of using a SimpleHttpClient to test behaviour of Expect/Continue handling
Comment 5 Christopher Schultz 2015-06-12 20:09:07 UTC
Having played-around with sun.net.http.allowRestrictedHeaders, I believe it's one of those things that gets consulted one time and then never looked-at again. That means that, if the tests are run in an unpredictable order, you'll never know which tests are using that value and which aren't. And you can't turn it off.

For predictability's sake, maybe we should set that system property for the whole set of unit tests to be run?

I think in the long run, use of SimpleHttpClient is probably better, since it will handle "Expect" the way we ... expect.
Comment 6 Violeta Georgieva 2015-06-16 07:52:20 UTC
I'm going to reimplement the TestAbstractHttp11Processor.testNon2xxResponseWithExpectation with SimpleHttpClient.

Regards,
Violeta
Comment 7 Violeta Georgieva 2015-06-18 11:15:41 UTC
Fix is available in Tomcat 7 trunk