Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

jetty-12 ee9 ServerPush failures #9766

Closed
janbartel opened this issue May 12, 2023 · 3 comments
Closed

jetty-12 ee9 ServerPush failures #9766

janbartel opened this issue May 12, 2023 · 3 comments
Assignees
Labels
Bug For general bugs on Jetty side TCK For various Specification Test Compatibility Kits (eg: Servlet, WebSocket, HTTP/2, etc)

Comments

@janbartel
Copy link
Contributor

Tck test: com/sun/ts/tests/servlet/spec/serverpush/Client.java.serverPushSessionTest2

This test sends an http/2 request to the server to create a session and push the index.html page back to the client. The test fails during the handling of pb.push() by never returning to the client.

There is one related TODO in the code: https://github.com/eclipse/jetty.project/blob/jetty-12.0.x/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/PushBuilderImpl.java#L177

The client code looks like:

        requestURI = "http://" + hostname + ":" + portnum + getContextRoot() + "/TestServlet5";
        Map<String, String> headers = new HashMap<>();
        CookieManager cm = new CookieManager();
        cm.setCookiePolicy(CookiePolicy.ACCEPT_ALL);
        List<HttpResponse<String>> responses = sendRequest(headers, null, cm);

        List<HttpCookie> cookies = cm.getCookieStore().get(new URI(
                "http://" + hostname + ":" + portnum + getContextRoot() + "/index.html"));
        for (HttpCookie cookie : cookies) {
            if ("JSESSIONID".equals(cookie.getName())) {
                pass = true;
                break;
            }
        }

The server code looks like:

    HttpSession session = req.getSession(true);
    pw.print(session.getId());
    PushBuilder pb = req.newPushBuilder();
    pb.path("index.html");
    pb.push();

Tck test: com/sun/ts/tests/servlet/spec/serverpush/Client.java.serverPushCookieTest

This test sends an http/2 request to the server, which creates a session plus some extra cookies and pushes the response back to the client. The test never finishes somewhere inside the handling of the pb.push() call.

The client code looks like:

        requestURI = "http://" + hostname + ":" + portnum + getContextRoot() +
                "/TestServlet4";
        Map<String, String> headers = new HashMap<>();
        headers.put("foo", "bar");
        CookieManager cm = new CookieManager();
        List<HttpResponse<String>> responses = sendRequest(headers, null, cm);
        verifyResponses(responses, new String[] { "add cookies [foo,bar] [baz,qux] to response", "INDEX from index.html" });

The server code looks like:

    Cookie cookie1 = new Cookie("foo", "bar");
    cookie1.setMaxAge(1000);
    resp.addCookie(cookie1);

    Cookie cookie2 = new Cookie("baz", "qux");
    cookie2.setMaxAge(-1);
    resp.addCookie(cookie2);
    pw.println("add cookies [foo,bar] [baz,qux] to response");

    HttpSession session = req.getSession(true);
    pw.println("create session: " + session);

    PushBuilder pb = req.newPushBuilder();
    pw.println("Cookie header in PushBuilder: " + pb.getHeader("Cookie"));
    pb.path("index.html");
    pb.push();
@janbartel janbartel added Bug For general bugs on Jetty side TCK For various Specification Test Compatibility Kits (eg: Servlet, WebSocket, HTTP/2, etc) Jetty 12 labels May 12, 2023
@sbordet
Copy link
Contributor

sbordet commented May 15, 2023

@janbartel for the first one, I'd say that should be enough to restore the TODO with:

            if (_request.isRequestedSessionIdFromURL())
                param = "jsessionid=" + _sessionId;
             else
                 _fields.put(HttpHeader.COOKIE,"JSESSIONID=" + _sessionId);

@sbordet
Copy link
Contributor

sbordet commented May 15, 2023

@janbartel the second looks like a broken test to me.
Given that the client does not send any cookie, the server should just push index.html with no cookies, so I'm not sure what's going on (as other simple pushes do not block like you report).

sbordet added a commit that referenced this issue May 16, 2023
* Restored dispatch=true for pushed requests.
* Restored tests that use the Servlet APIs to push.
* Ensured that pushed streams have request content EOF.

Signed-off-by: Simone Bordet <[email protected]>
sbordet added a commit that referenced this issue May 17, 2023
* Restored dispatch=true for pushed requests.
* Restored tests that use the Servlet APIs to push.
* Ensured that pushed streams have request content EOF.

Signed-off-by: Simone Bordet <[email protected]>
@janbartel
Copy link
Contributor Author

Fixed via #9776

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side TCK For various Specification Test Compatibility Kits (eg: Servlet, WebSocket, HTTP/2, etc)
Projects
None yet
Development

No branches or pull requests

2 participants