Bug 57154

Summary: Failure of TestWsWebSocketContainer when directory %TEMP%\test exists
Product: Tomcat 7 Reporter: Konstantin Kolinko <knst.kolinko>
Component: WebSocketAssignee: Tomcat Developers Mailing List <dev>
Severity: minor CC: huxing.zhang
Priority: P2    
Version: trunk   
Target Milestone: ---   
Hardware: PC   
OS: All   
Attachments: TEST-org.apache.tomcat.websocket.TestWsWebSocketContainer.NIO.txt
backport the feature from bug 57155 to Tomcat 7.0.x

Description Konstantin Kolinko 2014-10-27 22:07:01 UTC
Steps to reproduce:
1) Go to the temporary directory (System.getProperty("java.io.tmpdir")) and create a directory named "test" there.
2) Run org.apache.tomcat.websocket.TestWsWebSocketContainer test case with Nio or Nio 2.

The following two test cases are failing: 

Cause of the failure is the following ERROR:
The HTTP response from the server [HTTP/1.1 302 Found
] did not permit the HTTP upgrade to WebSocket
javax.websocket.DeploymentException: The HTTP response from the server [HTTP/1.1 302 Found
] did not permit the HTTP upgrade to WebSocket

This test case configures DefaultServlet and tries to access
"ws://localhost:" + getPort() + "/test".

I suspect that Mapper responds with 302 redirect before the upgrade request reaches web application.

This issue is observed at Buildbot. See
files in http://ci.apache.org/projects/tomcat/tomcat8/logs/1634329/
Comment 1 Konstantin Kolinko 2014-10-27 22:10:15 UTC
Created attachment 32152 [details]

Log file from testing Tomcat 8 trunk (@r1634690)
Comment 2 Konstantin Kolinko 2014-10-27 22:10:58 UTC
Created attachment 32153 [details]

Log file from testing Tomcat 8 trunk (@r1634690)
Comment 3 Mark Thomas 2014-10-29 15:01:15 UTC
The root cause of this is indeed the redirect from the Mapper.

The redirect is a result of one of the requirements for welcome file processing. The Servlet spec is clear Tomcat can implement these any way it likes so the current Tomcat behaviour is valid.

It looks like the best solution for this bug is a fix for bug 57155.
Comment 4 Mark Thomas 2014-10-29 22:22:02 UTC
Fixed with the fixes for bug 57155.
Comment 5 Konstantin Kolinko 2014-10-31 10:36:21 UTC
This test failure is reproducible with current Tomcat 7.0.x testsuite.
Note that it does not happen by default, but only if one creates a %TEMP%\test directory.

Possible solutions:
a) Backport the feature from bug 57155 (for reference: r1635222).
b) Reconfigure the tests to use some different temporary directory as docBase instead of the system-wide one.
c) Improve Mapper.
d) Close as WONTFIX.

a) is the best one, but requires some work as resource implementations differ between Tomcat 8 and 7.

b) is an alternative way if a) is too hard.

c) unlikely without further analysis.

d) possible. A test configuration issue is not a show stopper.

I am reopening this for Tomcat 7 now, so that it is not forgotten.
Comment 6 Huxing Zhang 2015-05-22 01:39:18 UTC
Created attachment 32749 [details]
backport the feature from bug 57155 to Tomcat 7.0.x

Hi @Konstantin and @Mark, 

I have backported the feature from bug 57155 to Tomcat 7.0.x.
Would you please kindly review the attached fix?
The main idea is borrowed from Mark's implementation in Tomcat 8.0.x.

The changes in the patch mainly includes:
1) an EmptyDirContext implementation which is not backed by a file system.
2) an enhancement in StandardContext that allows an application to be configured to have no docBase, precisely speaking, to have docBase to be null.
3) changes in test cases that replacing unnecessary file system doc base with null doc base.
Comment 7 Huxing Zhang 2015-05-27 01:26:25 UTC
Hi @Konstantin and @Mark, 

Is there any update on this?
Comment 8 Mark Thomas 2015-05-27 10:11:56 UTC
Thanks for the patch. It has been applied (with a few minor tweaks to silence some Eclipse warnings) to 7.0.x and will be included in 7.0.63 onwards.
Comment 9 Konstantin Kolinko 2016-01-17 04:06:04 UTC
(In reply to Mark Thomas from comment #8)
> Thanks for the patch. It has been applied (with a few minor tweaks to
> silence some Eclipse warnings) to 7.0.x and will be included in 7.0.63
> onwards.

For a reference, implementation of this feature in Tomcat 7 is
r1681953 + r1724913

Comments working on backport to Tomcat 6

1). Currently null is an unsupported value for StandardContext.docBase.

Using the null value is going to fail with a NullPointerException thrown
from a "new File(null)" call.

So implementing support for a null docBase is a new feature
and does not break any existing one.

2). There is a slight difference with Tomcat 7 that ApplicationContext and ReplicatedContext$ReplApplContext constructors accept additional argument basePath.

    public ServletContext getServletContext() {
        if (context == null) {
            context = new ApplicationContext(getBasePath(), this);

This resulted in a stacktrace like this:

     at java.io.File.<init>(File.java:194)
     at org.apache.catalina.core.StandardContext.getBasePath(StandardContext.java:5179)
     at org.apache.catalina.core.StandardContext.getServletContext(StandardContext.java:1932)
     at org.apache.catalina.core.StandardContext.postWorkDirectory(StandardContext.java:5448)
     at org.apache.catalina.core.StandardContext.start(StandardContext.java:4563)

The basePath value is used to implement methods ApplicationContext.getRealPath(String)
and getResource(String).

(For historic reference: commits that removed basePath field from ApplicationContext
in Tomcat 7 are r784083 + r834220)

In both methods the use of basePath is conditioned on "if (context.isFilesystemBased())"
condition. So if StandardContext is not based on a filesystem, the value of basePath in ApplicationContext does not matter.

Thus I am going to change StandardContext.getBasePath() to return null for a null docBase,
instead of failing with a NullPointerException.
Comment 10 Konstantin Kolinko 2016-01-17 05:17:44 UTC
Implemented in Tomcat 6 by r1725061 and will be in 6.0.45 onwards.