# HG changeset patch # User chegar # Date 1515862031 0 # Node ID b95b08f3e1a83fc8f7b56a2e6f65f9948732c8a2 # Parent c674ff28c69df9f72e2bf0584df4469ec09f2a6e 8194883: Unhandleable Push Promises should be cancelled Reviewed-by: dfuchs diff -r c674ff28c69d -r b95b08f3e1a8 src/jdk.incubator.httpclient/share/classes/jdk/incubator/http/Http2Connection.java --- a/src/jdk.incubator.httpclient/share/classes/jdk/incubator/http/Http2Connection.java Fri Jan 12 10:05:00 2018 -0800 +++ b/src/jdk.incubator.httpclient/share/classes/jdk/incubator/http/Http2Connection.java Sat Jan 13 16:47:11 2018 +0000 @@ -1,5 +1,5 @@ /* - * Copyright (c) 2015, 2017, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2015, 2018, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -558,6 +558,15 @@ } /** + * Streams initiated by a client MUST use odd-numbered stream + * identifiers; those initiated by the server MUST use even-numbered + * stream identifiers. + */ + private static final boolean isSeverInitiatedStream(int streamid) { + return (streamid & 0x1) == 0; + } + + /** * Handles stream 0 (common) frames that apply to whole connection and passes * other stream specific frames to that Stream object. * @@ -602,10 +611,19 @@ decodeHeaders((HeaderFrame) frame, decoder); } - int sid = frame.streamid(); - if (sid >= nextstreamid && !(frame instanceof ResetFrame)) { - // otherwise the stream has already been reset/closed - resetStream(streamid, ResetFrame.PROTOCOL_ERROR); + if (!(frame instanceof ResetFrame)) { + if (isSeverInitiatedStream(streamid)) { + if (streamid < nextPushStream) { + // trailing data on a cancelled push promise stream, + // reset will already have been sent, ignore + Log.logTrace("Ignoring cancelled push promise frame " + frame); + } else { + resetStream(streamid, ResetFrame.PROTOCOL_ERROR); + } + } else if (streamid >= nextstreamid) { + // otherwise the stream has already been reset/closed + resetStream(streamid, ResetFrame.PROTOCOL_ERROR); + } } return; } diff -r c674ff28c69d -r b95b08f3e1a8 src/jdk.incubator.httpclient/share/classes/jdk/incubator/http/Stream.java --- a/src/jdk.incubator.httpclient/share/classes/jdk/incubator/http/Stream.java Fri Jan 12 10:05:00 2018 -0800 +++ b/src/jdk.incubator.httpclient/share/classes/jdk/incubator/http/Stream.java Sat Jan 13 16:47:11 2018 +0000 @@ -1,5 +1,5 @@ /* - * Copyright (c) 2015, 2017, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2015, 2018, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -428,9 +428,10 @@ Log.logRequest("PUSH_PROMISE: " + pushReq.toString()); } PushGroup pushGroup = exchange.getPushGroup(); - if (pushGroup == null || pushGroup.noMorePushes()) { - cancelImpl(new IllegalStateException("unexpected push promise" - + " on stream " + streamid)); + if (pushGroup == null) { + Log.logTrace("Rejecting push promise stream " + streamid); + connection.resetStream(pushStream.streamid, ResetFrame.REFUSED_STREAM); + pushStream.close(); return; } diff -r c674ff28c69d -r b95b08f3e1a8 test/jdk/java/net/httpclient/http2/ImplicitPushCancel.java --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/test/jdk/java/net/httpclient/http2/ImplicitPushCancel.java Sat Jan 13 16:47:11 2018 +0000 @@ -0,0 +1,176 @@ +/* + * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + */ + +/* + * @test + * @library /lib/testlibrary server + * @build jdk.testlibrary.SimpleSSLContext + * @modules java.base/sun.net.www.http + * jdk.incubator.httpclient/jdk.incubator.http.internal.common + * jdk.incubator.httpclient/jdk.incubator.http.internal.frame + * jdk.incubator.httpclient/jdk.incubator.http.internal.hpack + * @run testng/othervm -Djdk.internal.httpclient.debug=true -Djdk.httpclient.HttpClient.log=errors,requests,responses,trace ImplicitPushCancel + */ + +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.net.URI; +import java.util.Map; +import java.util.Objects; +import java.util.Optional; +import jdk.incubator.http.HttpClient; +import jdk.incubator.http.HttpRequest; +import jdk.incubator.http.HttpResponse; +import jdk.incubator.http.HttpResponse.BodyHandler; +import jdk.incubator.http.MultiMapResult; +import jdk.incubator.http.internal.common.HttpHeadersImpl; +import org.testng.annotations.AfterTest; +import org.testng.annotations.BeforeTest; +import org.testng.annotations.Test; +import static java.nio.charset.StandardCharsets.UTF_8; +import static org.testng.Assert.assertEquals; + +public class ImplicitPushCancel { + + static Map PUSH_PROMISES = Map.of( + "/x/y/z/1", "the first push promise body", + "/x/y/z/2", "the second push promise body", + "/x/y/z/3", "the third push promise body", + "/x/y/z/4", "the fourth push promise body", + "/x/y/z/5", "the fifth push promise body", + "/x/y/z/6", "the sixth push promise body", + "/x/y/z/7", "the seventh push promise body", + "/x/y/z/8", "the eight push promise body", + "/x/y/z/9", "the ninth push promise body" + ); + static final String MAIN_RESPONSE_BODY = "the main response body"; + + Http2TestServer server; + URI uri; + + @BeforeTest + public void setup() throws Exception { + server = new Http2TestServer(false, 0); + Http2Handler handler = new ServerPushHandler(MAIN_RESPONSE_BODY, + PUSH_PROMISES); + server.addHandler(handler, "/"); + server.start(); + int port = server.getAddress().getPort(); + System.err.println("Server listening on port " + port); + uri = new URI("http://127.0.0.1:" + port + "/foo/a/b/c"); + } + + @AfterTest + public void teardown() { + server.stop(); + } + + static final HttpResponse assert200ResponseCode(HttpResponse response) { + assertEquals(response.statusCode(), 200); + return response; + } + + /* + * With a handler not capable of accepting push promises, then all push + * promises should be rejected / cancelled, without interfering with the + * main response. + */ + @Test + public void test() throws Exception { + HttpClient client = HttpClient.newHttpClient(); + + client.sendAsync(HttpRequest.newBuilder(uri).build(), BodyHandler.asString()) + .thenApply(ImplicitPushCancel::assert200ResponseCode) + .thenApply(HttpResponse::body) + .thenAccept(body -> body.equals(MAIN_RESPONSE_BODY)) + .join(); + + MultiMapResult map = client.sendAsync( + HttpRequest.newBuilder(uri).build(), + HttpResponse.MultiSubscriber.asMap( + (req) -> Optional.of(HttpResponse.BodyHandler.asString())) + ).join(); + + map.entrySet().stream().forEach(e -> System.out.println(e.getKey() + ":" + e.getValue().join().body())); + + map.entrySet().stream().forEach(entry -> { + HttpRequest request = entry.getKey(); + HttpResponse response = entry.getValue().join(); + assertEquals(response.statusCode(), 200); + if (PUSH_PROMISES.containsKey(request.uri().getPath())) { + assertEquals(response.body(), PUSH_PROMISES.get(request.uri().getPath())); + } else { + assertEquals(response.body(), MAIN_RESPONSE_BODY); + } + + } ); + } + + // --- server push handler --- + static class ServerPushHandler implements Http2Handler { + + private final String mainResponseBody; + private final Map promises; + + public ServerPushHandler(String mainResponseBody, + Map promises) + throws Exception + { + Objects.requireNonNull(promises); + this.mainResponseBody = mainResponseBody; + this.promises = promises; + } + + public void handle(Http2TestExchange exchange) throws IOException { + System.err.println("Server: handle " + exchange); + try (InputStream is = exchange.getRequestBody()) { + is.readAllBytes(); + } + + if (exchange.serverPushAllowed()) { + pushPromises(exchange); + } + + // response data for the main response + try (OutputStream os = exchange.getResponseBody()) { + byte[] bytes = mainResponseBody.getBytes(UTF_8); + exchange.sendResponseHeaders(200, bytes.length); + os.write(bytes); + } + } + + private void pushPromises(Http2TestExchange exchange) throws IOException { + URI requestURI = exchange.getRequestURI(); + for (Map.Entry promise : promises.entrySet()) { + URI uri = requestURI.resolve(promise.getKey()); + InputStream is = new ByteArrayInputStream(promise.getValue().getBytes(UTF_8)); + HttpHeadersImpl headers = new HttpHeadersImpl(); + exchange.serverPush(uri, headers, is); + } + System.err.println("Server: All pushes sent"); + } + } +} + diff -r c674ff28c69d -r b95b08f3e1a8 test/jdk/java/net/httpclient/http2/server/Http2TestServerConnection.java --- a/test/jdk/java/net/httpclient/http2/server/Http2TestServerConnection.java Fri Jan 12 10:05:00 2018 -0800 +++ b/test/jdk/java/net/httpclient/http2/server/Http2TestServerConnection.java Sat Jan 13 16:47:11 2018 +0000 @@ -1,5 +1,5 @@ /* - * Copyright (c) 2015, 2017, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2015, 2018, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -643,11 +643,21 @@ // This should mean depending on what the // handler is doing: either an EOF on read // or an IOException if writing the response. - q.orderlyClose(); - BodyOutputStream oq = outStreams.get(stream); - if (oq != null) - oq.closeInternal(); - + if (q != null) { + q.orderlyClose(); + BodyOutputStream oq = outStreams.get(stream); + if (oq != null) + oq.closeInternal(); + } else if (pushStreams.contains(stream)) { + // we could interrupt the pushStream's output + // but the continuation, even after a reset + // should be handle gracefully by the client + // anyway. + } else { + System.err.println("TestServer: Unexpected frame on: " + stream); + System.err.println(frame); + throw new IOException("Unexpected frame"); + } } else { q.put(frame); }