Skip to content

Commit 72294c5

Browse files
dfuchdfuch
committed
8308024: HttpClient (HTTP/1.1) sends an extraneous empty chunk if the BodyPublisher supplies an empty buffer
Reviewed-by: djelinski, michaelm
1 parent c9b6bb5 commit 72294c5

File tree

5 files changed

+417
-171
lines changed

5 files changed

+417
-171
lines changed

‎src/java.net.http/share/classes/jdk/internal/net/http/Http1Request.java

+12-5
Original file line numberDiff line numberDiff line change
@@ -350,11 +350,18 @@ public void onNext(ByteBuffer item) {
350350
http1Exchange.appendToOutgoing(t);
351351
} else {
352352
int chunklen = item.remaining();
353-
ArrayList<ByteBuffer> l = new ArrayList<>(3);
354-
l.add(getHeader(chunklen));
355-
l.add(item);
356-
l.add(ByteBuffer.wrap(CRLF));
357-
http1Exchange.appendToOutgoing(l);
353+
if (chunklen > 0) {
354+
ArrayList<ByteBuffer> l = new ArrayList<>(3);
355+
l.add(getHeader(chunklen));
356+
l.add(item);
357+
l.add(ByteBuffer.wrap(CRLF));
358+
http1Exchange.appendToOutgoing(l);
359+
} else {
360+
if (debug.on()) {
361+
debug.log("dropping empty buffer, request one more");
362+
}
363+
request(1);
364+
}
358365
}
359366
}
360367

‎test/jdk/java/net/httpclient/AbstractNoBody.java

+153-80
Original file line numberDiff line numberDiff line change
@@ -21,35 +21,43 @@
2121
* questions.
2222
*/
2323

24+
import java.io.Closeable;
2425
import java.io.IOException;
2526
import java.io.InputStream;
26-
import java.net.InetAddress;
27-
import java.net.InetSocketAddress;
28-
import java.util.concurrent.Executor;
27+
import java.net.URI;
28+
import java.net.http.HttpClient.Version;
29+
import java.net.http.HttpRequest;
30+
import java.net.http.HttpResponse;
31+
import java.net.http.HttpResponse.BodyHandlers;
32+
import java.nio.ByteBuffer;
33+
import java.nio.charset.Charset;
34+
import java.nio.charset.StandardCharsets;
2935
import java.util.concurrent.ExecutorService;
3036
import java.util.concurrent.Executors;
31-
import com.sun.net.httpserver.HttpExchange;
32-
import com.sun.net.httpserver.HttpHandler;
33-
import com.sun.net.httpserver.HttpServer;
34-
import com.sun.net.httpserver.HttpsConfigurator;
35-
import com.sun.net.httpserver.HttpsServer;
3637
import java.net.http.HttpClient;
38+
import java.util.concurrent.atomic.AtomicLong;
3739
import javax.net.ssl.SSLContext;
38-
import jdk.httpclient.test.lib.http2.Http2TestServer;
39-
import jdk.httpclient.test.lib.http2.Http2TestExchange;
40-
import jdk.httpclient.test.lib.http2.Http2Handler;
40+
41+
import jdk.httpclient.test.lib.common.HttpServerAdapters;
4142
import jdk.test.lib.net.SimpleSSLContext;
4243
import org.testng.annotations.AfterTest;
4344
import org.testng.annotations.BeforeTest;
4445
import org.testng.annotations.DataProvider;
4546

46-
public abstract class AbstractNoBody {
47+
import static java.lang.System.err;
48+
import static java.lang.System.out;
49+
import static java.net.http.HttpClient.Builder.NO_PROXY;
50+
import static java.net.http.HttpClient.Version.HTTP_1_1;
51+
import static java.net.http.HttpClient.Version.HTTP_2;
52+
import static org.testng.Assert.assertEquals;
53+
54+
public abstract class AbstractNoBody implements HttpServerAdapters {
4755

4856
SSLContext sslContext;
49-
HttpServer httpTestServer; // HTTP/1.1 [ 4 servers ]
50-
HttpsServer httpsTestServer; // HTTPS/1.1
51-
Http2TestServer http2TestServer; // HTTP/2 ( h2c )
52-
Http2TestServer https2TestServer; // HTTP/2 ( h2 )
57+
HttpTestServer httpTestServer; // HTTP/1.1 [ 4 servers ]
58+
HttpTestServer httpsTestServer; // HTTPS/1.1
59+
HttpTestServer http2TestServer; // HTTP/2 ( h2c )
60+
HttpTestServer https2TestServer; // HTTP/2 ( h2 )
5361
String httpURI_fixed;
5462
String httpURI_chunk;
5563
String httpsURI_fixed;
@@ -62,8 +70,17 @@ public abstract class AbstractNoBody {
6270
static final String SIMPLE_STRING = "Hello world. Goodbye world";
6371
static final int ITERATION_COUNT = 3;
6472
// a shared executor helps reduce the amount of threads created by the test
65-
static final Executor executor = Executors.newFixedThreadPool(ITERATION_COUNT * 2);
73+
static final ExecutorService executor = Executors.newFixedThreadPool(ITERATION_COUNT * 2);
6674
static final ExecutorService serverExecutor = Executors.newFixedThreadPool(ITERATION_COUNT * 4);
75+
static final AtomicLong clientCount = new AtomicLong();
76+
static final long start = System.nanoTime();
77+
public static String now() {
78+
long now = System.nanoTime() - start;
79+
long secs = now / 1000_000_000;
80+
long mill = (now % 1000_000_000) / 1000_000;
81+
long nan = now % 1000_000;
82+
return String.format("[%d s, %d ms, %d ns] ", secs, mill, nan);
83+
}
6784

6885
@DataProvider(name = "variants")
6986
public Object[][] variants() {
@@ -88,55 +105,86 @@ public Object[][] variants() {
88105
};
89106
}
90107

91-
HttpClient newHttpClient() {
108+
private volatile HttpClient sharedClient;
109+
110+
static Version version(String uri) {
111+
if (uri.contains("/http1/") || uri.contains("/https1/"))
112+
return HTTP_1_1;
113+
if (uri.contains("/http2/") || uri.contains("/https2/"))
114+
return HTTP_2;
115+
return null;
116+
}
117+
118+
HttpRequest.Builder newRequestBuilder(String uri) {
119+
var builder = HttpRequest.newBuilder(URI.create(uri));
120+
return builder;
121+
}
122+
123+
private HttpClient makeNewClient() {
124+
clientCount.incrementAndGet();
92125
return HttpClient.newBuilder()
93126
.executor(executor)
127+
.proxy(NO_PROXY)
94128
.sslContext(sslContext)
95129
.build();
96130
}
97131

98-
static String serverAuthority(HttpServer server) {
99-
return InetAddress.getLoopbackAddress().getHostName() + ":"
100-
+ server.getAddress().getPort();
132+
HttpClient newHttpClient(boolean share) {
133+
if (!share) return makeNewClient();
134+
HttpClient shared = sharedClient;
135+
if (shared != null) return shared;
136+
synchronized (this) {
137+
shared = sharedClient;
138+
if (shared == null) {
139+
shared = sharedClient = makeNewClient();
140+
}
141+
return shared;
142+
}
143+
}
144+
145+
record CloseableClient(HttpClient client, boolean shared)
146+
implements Closeable {
147+
public void close() {
148+
if (shared) return;
149+
client.close();
150+
}
101151
}
102152

103153
@BeforeTest
104154
public void setup() throws Exception {
105155
printStamp(START, "setup");
156+
HttpServerAdapters.enableServerLogging();
106157
sslContext = new SimpleSSLContext().get();
107158
if (sslContext == null)
108159
throw new AssertionError("Unexpected null sslContext");
109160

110161
// HTTP/1.1
111-
HttpHandler h1_fixedLengthNoBodyHandler = new HTTP1_FixedLengthNoBodyHandler();
112-
HttpHandler h1_chunkNoBodyHandler = new HTTP1_ChunkedNoBodyHandler();
113-
InetSocketAddress sa = new InetSocketAddress(InetAddress.getLoopbackAddress(), 0);
114-
httpTestServer = HttpServer.create(sa, 0);
115-
httpTestServer.setExecutor(serverExecutor);
116-
httpTestServer.createContext("/http1/noBodyFixed", h1_fixedLengthNoBodyHandler);
117-
httpTestServer.createContext("/http1/noBodyChunk", h1_chunkNoBodyHandler);
118-
httpURI_fixed = "http://" + serverAuthority(httpTestServer) + "/http1/noBodyFixed";
119-
httpURI_chunk = "http://" + serverAuthority(httpTestServer) + "/http1/noBodyChunk";
120-
121-
httpsTestServer = HttpsServer.create(sa, 0);
122-
httpsTestServer.setExecutor(serverExecutor);
123-
httpsTestServer.setHttpsConfigurator(new HttpsConfigurator(sslContext));
124-
httpsTestServer.createContext("/https1/noBodyFixed", h1_fixedLengthNoBodyHandler);
125-
httpsTestServer.createContext("/https1/noBodyChunk", h1_chunkNoBodyHandler);
126-
httpsURI_fixed = "https://" + serverAuthority(httpsTestServer) + "/https1/noBodyFixed";
127-
httpsURI_chunk = "https://" + serverAuthority(httpsTestServer) + "/https1/noBodyChunk";
162+
HttpTestHandler h1_fixedLengthNoBodyHandler = new FixedLengthNoBodyHandler();
163+
HttpTestHandler h1_chunkNoBodyHandler = new ChunkedNoBodyHandler();
164+
165+
httpTestServer = HttpTestServer.create(HTTP_1_1, null, serverExecutor);
166+
httpTestServer.addHandler(h1_fixedLengthNoBodyHandler,"/http1/noBodyFixed");
167+
httpTestServer.addHandler(h1_chunkNoBodyHandler, "/http1/noBodyChunk");
168+
httpURI_fixed = "http://" + httpTestServer.serverAuthority() + "/http1/noBodyFixed";
169+
httpURI_chunk = "http://" + httpTestServer.serverAuthority() + "/http1/noBodyChunk";
170+
171+
httpsTestServer = HttpTestServer.create(HTTP_1_1, sslContext, serverExecutor);
172+
httpsTestServer.addHandler(h1_fixedLengthNoBodyHandler,"/https1/noBodyFixed");
173+
httpsTestServer.addHandler(h1_chunkNoBodyHandler, "/https1/noBodyChunk");
174+
httpsURI_fixed = "https://" + httpsTestServer.serverAuthority() + "/https1/noBodyFixed";
175+
httpsURI_chunk = "https://" + httpsTestServer.serverAuthority() + "/https1/noBodyChunk";
128176

129177
// HTTP/2
130-
Http2Handler h2_fixedLengthNoBodyHandler = new HTTP2_FixedLengthNoBodyHandler();
131-
Http2Handler h2_chunkedNoBodyHandler = new HTTP2_ChunkedNoBodyHandler();
178+
HttpTestHandler h2_fixedLengthNoBodyHandler = new FixedLengthNoBodyHandler();
179+
HttpTestHandler h2_chunkedNoBodyHandler = new ChunkedNoBodyHandler();
132180

133-
http2TestServer = new Http2TestServer("localhost", false, 0, serverExecutor, null);
181+
http2TestServer = HttpTestServer.create(HTTP_2, null, serverExecutor);
134182
http2TestServer.addHandler(h2_fixedLengthNoBodyHandler, "/http2/noBodyFixed");
135183
http2TestServer.addHandler(h2_chunkedNoBodyHandler, "/http2/noBodyChunk");
136184
http2URI_fixed = "http://" + http2TestServer.serverAuthority() + "/http2/noBodyFixed";
137185
http2URI_chunk = "http://" + http2TestServer.serverAuthority() + "/http2/noBodyChunk";
138186

139-
https2TestServer = new Http2TestServer("localhost", true, 0, serverExecutor, sslContext);
187+
https2TestServer = HttpTestServer.create(HTTP_2, sslContext, serverExecutor);
140188
https2TestServer.addHandler(h2_fixedLengthNoBodyHandler, "/https2/noBodyFixed");
141189
https2TestServer.addHandler(h2_chunkedNoBodyHandler, "/https2/noBodyChunk");
142190
https2URI_fixed = "https://" + https2TestServer.serverAuthority() + "/https2/noBodyFixed";
@@ -146,77 +194,102 @@ public void setup() throws Exception {
146194
httpsTestServer.start();
147195
http2TestServer.start();
148196
https2TestServer.start();
197+
198+
var shared = newHttpClient(true);
199+
200+
out.println("HTTP/1.1 server (http) listening at: " + httpTestServer.serverAuthority());
201+
out.println("HTTP/1.1 server (TLS) listening at: " + httpsTestServer.serverAuthority());
202+
out.println("HTTP/2 server (h2c) listening at: " + http2TestServer.serverAuthority());
203+
out.println("HTTP/2 server (h2) listening at: " + https2TestServer.serverAuthority());
204+
205+
out.println("Shared client is: " + shared);
206+
149207
printStamp(END,"setup");
150208
}
151209

152210
@AfterTest
153211
public void teardown() throws Exception {
154212
printStamp(START, "teardown");
155-
httpTestServer.stop(0);
156-
httpsTestServer.stop(0);
213+
sharedClient.close();
214+
httpTestServer.stop();
215+
httpsTestServer.stop();
157216
http2TestServer.stop();
158217
https2TestServer.stop();
218+
executor.close();
219+
serverExecutor.close();
159220
printStamp(END, "teardown");
160221
}
161222

162-
static final long start = System.nanoTime();
163223
static final String START = "start";
164224
static final String END = "end ";
165-
static long elapsed() { return (System.nanoTime() - start)/1000_000;}
166225
void printStamp(String what, String fmt, Object... args) {
167-
long elapsed = elapsed();
168-
long sec = elapsed/1000;
169-
long ms = elapsed % 1000;
170-
String time = sec > 0 ? sec + "sec " : "";
171-
time = time + ms + "ms";
172226
System.out.printf("%s: %s \t [%s]\t %s%n",
173-
getClass().getSimpleName(), what, time, String.format(fmt,args));
227+
getClass().getSimpleName(), what, now(), String.format(fmt,args));
174228
}
175229

176230

177-
static class HTTP1_FixedLengthNoBodyHandler implements HttpHandler {
231+
static class FixedLengthNoBodyHandler implements HttpTestHandler {
178232
@Override
179-
public void handle(HttpExchange t) throws IOException {
233+
public void handle(HttpTestExchange t) throws IOException {
180234
//out.println("NoBodyHandler received request to " + t.getRequestURI());
235+
boolean echo = "echo".equals(t.getRequestURI().getRawQuery());
236+
byte[] reqbytes;
181237
try (InputStream is = t.getRequestBody()) {
182-
is.readAllBytes();
238+
reqbytes = is.readAllBytes();
239+
}
240+
if (echo) {
241+
t.sendResponseHeaders(200, reqbytes.length);
242+
if (reqbytes.length > 0) {
243+
try (var os = t.getResponseBody()) {
244+
os.write(reqbytes);
245+
}
246+
}
247+
} else {
248+
t.sendResponseHeaders(200, 0); // no body
183249
}
184-
t.sendResponseHeaders(200, -1); // no body
185250
}
186251
}
187252

188-
static class HTTP1_ChunkedNoBodyHandler implements HttpHandler {
253+
static class ChunkedNoBodyHandler implements HttpTestHandler {
189254
@Override
190-
public void handle(HttpExchange t) throws IOException {
255+
public void handle(HttpTestExchange t) throws IOException {
191256
//out.println("NoBodyHandler received request to " + t.getRequestURI());
257+
boolean echo = "echo".equals(t.getRequestURI().getRawQuery());
258+
byte[] reqbytes;
192259
try (InputStream is = t.getRequestBody()) {
193-
is.readAllBytes();
260+
reqbytes = is.readAllBytes();
261+
}
262+
if (echo) {
263+
t.sendResponseHeaders(200, -1);
264+
try (var os = t.getResponseBody()) {
265+
os.write(reqbytes);
266+
}
267+
} else {
268+
t.sendResponseHeaders(200, -1); // chunked
269+
t.getResponseBody().close(); // write nothing
194270
}
195-
t.sendResponseHeaders(200, 0); // chunked
196-
t.getResponseBody().close(); // write nothing
197271
}
198272
}
199273

200-
static class HTTP2_FixedLengthNoBodyHandler implements Http2Handler {
201-
@Override
202-
public void handle(Http2TestExchange t) throws IOException {
203-
//out.println("NoBodyHandler received request to " + t.getRequestURI());
204-
try (InputStream is = t.getRequestBody()) {
205-
is.readAllBytes();
206-
}
207-
t.sendResponseHeaders(200, 0);
208-
}
274+
/*
275+
* Converts a ByteBuffer containing bytes encoded using
276+
* the given charset into a string.
277+
* This method does not throw but will replace
278+
* unrecognized sequences with the replacement character.
279+
*/
280+
public static String asString(ByteBuffer buffer, Charset charset) {
281+
var decoded = charset.decode(buffer);
282+
char[] chars = new char[decoded.length()];
283+
decoded.get(chars);
284+
return new String(chars);
209285
}
210286

211-
static class HTTP2_ChunkedNoBodyHandler implements Http2Handler {
212-
@Override
213-
public void handle(Http2TestExchange t) throws IOException {
214-
//out.println("NoBodyHandler received request to " + t.getRequestURI());
215-
try (InputStream is = t.getRequestBody()) {
216-
is.readAllBytes();
217-
}
218-
t.sendResponseHeaders(200, -1);
219-
t.getResponseBody().close(); // write nothing
220-
}
287+
/*
288+
* Converts a ByteBuffer containing UTF-8 bytes into a
289+
* string. This method does not throw but will replace
290+
* unrecognized sequences with the replacement character.
291+
*/
292+
public static String asString(ByteBuffer buffer) {
293+
return asString(buffer, StandardCharsets.UTF_8);
221294
}
222295
}

0 commit comments

Comments
 (0)