Skip to content

Commit

Permalink
Jetty 10.0.x immutable meta data (#4777)
Browse files Browse the repository at this point in the history 
Made HttpURI, HttpFields and MetaData immutable.  The first two follow the same builder pattern and MetaData is constructor injection only.

* Immutable version of HttpFields

Preserve API and usage of HttpFields class while providing a read only interface and immutable implementation.

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Immutable version of HttpFields

Use an ArrayList in HttpFields. While slightly slower than the array, it will mostly be used as a builder pattern for an Immutable

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Immutable version of HttpFields

Fixed exception type.

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Immutable version of HttpFields

asImmutable method

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Immutable MetaData

Made HttpURIU immutable with a builder pattern.
MetaData immutable and working within http module.

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Immutable MetaData

Fixes from review

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Immutable MetaData

Passing tests upto and including jetty-server

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Immutable MetaData

Cleanup of HttpURI.Builder API as suggested in PR.

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Immutable MetaData

Added builder for MetaData.Request

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Immutable MetaData

more api fixes

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Immutable MetaData

WIP making HttpFiels itself immutable.  Currently working up to jetty-servlet.

Need to consider if content-length really is meta data and how much and when can we trust it.

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Immutable MetaData

WIP

Need to consider if content-length really is meta data and how much and when can we trust it. Also need to consider difference between h2 and h1 authority in metadata.

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Immutable MetaData WIP

jetty-client and jetty-servlet passing tests.

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Immutable MetaData WIP

Better align the style of immutability between `HttpFields` and `HttpURI`.
They both now have static build() and from() methods, plus Builder and Immutable implementations.
Potentially `Builder` could be renamed as `Mutable`

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Immutable MetaData WIP

http2-server tests passed

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Immutable MetaData WIP

http2-client tests passed

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Immutable MetaData WIP

cleann build?

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Immutable MetaData WIP

fix

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Immutable MetaData WIP

more test fixes

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Immutable MetaData WIP

Cleanups, mostly using EMPTY when appropriate.

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Immutable MetaData WIP

Cleanups, use immutable

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Immutable MetaData WIP

No trailers for connect

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Immutable MetaData WIP

Fix CONNECT path handling

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Immutable MetaData WIP

fixed rewrite query handling

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Immutable MetaData WIP

rename Builders to Muttables

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Immutable MetaData WIP

misc cleanups

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Immutable MetaData WIP

Revert to using arrays due to garbage generated by streams and iterators (12% of a simple benchmark!).
Even if this garbage is an artifact of the JIT being disabled by observation, it can hide other allocations, so best to just use simple arrays!

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Immutable MetaData WIP

More optimizations and better test coverage.

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Immutable Metadata

various cleanups

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Immutable MetaData WIP

More optimizations

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Immutable MetaData WIP

review changes

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Immutable MetaData WIP

changes after review:
 + less usage of Mutable
 + more usage of EMPTY
 + restored fragment handling

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Immutable MetaData WIP

changes after review:
 + less usage of Mutable
 + less usage of asImmutable

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Immutable MetaData WIP

changes after review:
 + less usage of Mutable

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Immutable MetaData

changes after review:
 + better handling of URI in ContextHandler

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Immutable MetaData

changes after review:
 + downcast in test to access mutable response headers.

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Immutable MetaData

changes after review:
 + use put instead of add for one time headers

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* private

Signed-off-by: Greg Wilkins <gregw@webtide.com>
  • Loading branch information
@gregw
gregw committed Apr 28, 2020
1 parent 4bf80d9 commit 8c7e34f
Show file tree
Hide file tree
Showing 148 changed files with 3,883 additions and 3,279 deletions.
5 changes: 2 additions & 3 deletions ...ypt-client/src/test/java/org/eclipse/jetty/alpn/java/client/ConscryptHTTP2ClientTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,8 @@ public void testConscryptHTTP2Client() throws Exception
client.connect(sslContextFactory, new InetSocketAddress(host, port), new Session.Listener.Adapter(), sessionPromise);
Session session = sessionPromise.get(15, TimeUnit.SECONDS);

HttpFields requestFields = new HttpFields();
requestFields.put("User-Agent", client.getClass().getName() + "/" + Jetty.VERSION);
MetaData.Request metaData = new MetaData.Request("GET", new HttpURI("https://" + host + ":" + port + "/"), HttpVersion.HTTP_2, requestFields);
HttpFields requestFields = HttpFields.build().put("User-Agent", client.getClass().getName() + "/" + Jetty.VERSION);
MetaData.Request metaData = new MetaData.Request("GET", HttpURI.from("https://" + host + ":" + port + "/"), HttpVersion.HTTP_2, requestFields);
HeadersFrame headersFrame = new HeadersFrame(metaData, null, true);
CountDownLatch latch = new CountDownLatch(1);
session.newStream(headersFrame, new Promise.Adapter<>(), new Stream.Listener.Adapter()
Expand Down
4 changes: 2 additions & 2 deletions ...lpn-java-client/src/test/java/org/eclipse/jetty/alpn/java/client/JDK9HTTP2ClientTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,9 @@ public void testJDK9HTTP2Client() throws Exception
client.connect(sslContextFactory, new InetSocketAddress(host, port), new Session.Listener.Adapter(), sessionPromise);
Session session = sessionPromise.get(15, TimeUnit.SECONDS);

HttpFields requestFields = new HttpFields();
HttpFields.Mutable requestFields = HttpFields.build();
requestFields.put("User-Agent", client.getClass().getName() + "/" + Jetty.VERSION);
MetaData.Request metaData = new MetaData.Request("GET", new HttpURI("https://" + host + ":" + port + "/"), HttpVersion.HTTP_2, requestFields);
MetaData.Request metaData = new MetaData.Request("GET", HttpURI.from("https://" + host + ":" + port + "/"), HttpVersion.HTTP_2, requestFields);
HeadersFrame headersFrame = new HeadersFrame(metaData, null, true);
CountDownLatch latch = new CountDownLatch(1);
session.newStream(headersFrame, new Promise.Adapter<>(), new Stream.Listener.Adapter()
Expand Down
2 changes: 1 addition & 1 deletion jetty-client/src/main/java/org/eclipse/jetty/client/AuthenticationProtocolHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,7 @@ private void copyIfAbsent(HttpRequest oldRequest, Request newRequest, HttpHeader
{
HttpField field = oldRequest.getHeaders().getField(header);
if (field != null && !newRequest.getHeaders().contains(header))
newRequest.getHeaders().put(field);
newRequest.put(field);
}

private void forwardSuccessComplete(HttpRequest request, Response response)
Expand Down
17 changes: 9 additions & 8 deletions jetty-client/src/main/java/org/eclipse/jetty/client/HttpConnection.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import org.eclipse.jetty.client.api.Request;
import org.eclipse.jetty.client.api.Response;
import org.eclipse.jetty.client.util.BytesRequestContent;
import org.eclipse.jetty.http.HttpField;
import org.eclipse.jetty.http.HttpFields;
import org.eclipse.jetty.http.HttpHeader;
import org.eclipse.jetty.http.HttpVersion;
Expand Down Expand Up @@ -151,8 +152,8 @@ protected void normalizeRequest(Request request)
HttpFields headers = request.getHeaders();
if (version.getVersion() <= 11)
{
if (!headers.containsKey(HttpHeader.HOST.asString()))
headers.put(getHttpDestination().getHostField());
if (!headers.contains(HttpHeader.HOST))
request.put(getHttpDestination().getHostField());
}

// Add content headers
Expand All @@ -163,25 +164,25 @@ protected void normalizeRequest(Request request)
}
else
{
if (!headers.containsKey(HttpHeader.CONTENT_TYPE.asString()))
if (!headers.contains(HttpHeader.CONTENT_TYPE))
{
String contentType = content.getContentType();
if (contentType != null)
{
headers.put(HttpHeader.CONTENT_TYPE, contentType);
request.put(new HttpField(HttpHeader.CONTENT_TYPE, contentType));
}
else
{
contentType = getHttpClient().getDefaultRequestContentType();
if (contentType != null)
headers.put(HttpHeader.CONTENT_TYPE, contentType);
request.put(new HttpField(HttpHeader.CONTENT_TYPE, contentType));
}
}
long contentLength = content.getLength();
if (contentLength >= 0)
{
if (!headers.containsKey(HttpHeader.CONTENT_LENGTH.asString()))
headers.put(HttpHeader.CONTENT_LENGTH, String.valueOf(contentLength));
if (!headers.contains(HttpHeader.CONTENT_LENGTH))
request.put(new HttpField.LongValueHttpField(HttpHeader.CONTENT_LENGTH, contentLength));
}
}

Expand All @@ -194,7 +195,7 @@ protected void normalizeRequest(Request request)
cookies = convertCookies(HttpCookieStore.matchPath(uri, cookieStore.get(uri)), null);
cookies = convertCookies(request.getCookies(), cookies);
if (cookies != null)
request.header(HttpHeader.COOKIE.asString(), cookies.toString());
request.header(HttpHeader.COOKIE, cookies.toString());
}

// Authentication
Expand Down
2 changes: 1 addition & 1 deletion jetty-client/src/main/java/org/eclipse/jetty/client/HttpReceiver.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ protected boolean responseHeader(HttpExchange exchange, HttpField field)
boolean process = notifier.notifyHeader(exchange.getConversation().getResponseListeners(), response, field);
if (process)
{
response.getHeaders().add(field);
response.getHeaderFieldsMutable().add(field);
HttpHeader fieldHeader = field.getHeader();
if (fieldHeader != null)
{
Expand Down
32 changes: 30 additions & 2 deletions jetty-client/src/main/java/org/eclipse/jetty/client/HttpRequest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ public class HttpRequest implements Request
{
private static final URI NULL_URI = URI.create("null:0");

private final HttpFields headers = new HttpFields();
private final HttpFields.Mutable headers = HttpFields.build();
private final Fields params = new Fields(true);
private final List<Response.ResponseListener> responseListeners = new ArrayList<>();
private final AtomicReference<Throwable> aborted = new AtomicReference<>();
Expand Down Expand Up @@ -305,6 +305,34 @@ public Request accept(String... accepts)
return this;
}

@Override
public Request set(HttpFields fields)
{
headers.clear().add(fields);
return this;
}

@Override
public Request remove(HttpHeader header)
{
headers.remove(header);
return this;
}

@Override
public Request put(HttpField field)
{
headers.put(field);
return this;
}

@Override
public Request add(HttpField field)
{
headers.add(field);
return this;
}

@Override
public Request header(String name, String value)
{
Expand Down Expand Up @@ -369,7 +397,7 @@ public Map<String, Object> getAttributes()
}

@Override
public HttpFields getHeaders()
public HttpFields.Mutable getHeaders()
{
return headers;
}
Expand Down
11 changes: 8 additions & 3 deletions jetty-client/src/main/java/org/eclipse/jetty/client/HttpResponse.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,13 @@

public class HttpResponse implements Response
{
private final HttpFields headers = new HttpFields();
private final HttpFields.Mutable headers = HttpFields.build();
private final Request request;
private final List<ResponseListener> listeners;
private HttpVersion version;
private int status;
private String reason;
private HttpFields trailers;
private HttpFields.Mutable trailers;

public HttpResponse(Request request, List<ResponseListener> listeners)
{
Expand Down Expand Up @@ -87,6 +87,11 @@ public HttpResponse reason(String reason)

@Override
public HttpFields getHeaders()
{
return headers.asImmutable();
}

public HttpFields.Mutable getHeaderFieldsMutable()
{
return headers;
}
Expand All @@ -111,7 +116,7 @@ public HttpFields getTrailers()
public HttpResponse trailer(HttpField trailer)
{
if (trailers == null)
trailers = new HttpFields();
trailers = HttpFields.build();
trailers.add(trailer);
return this;
}
Expand Down
27 changes: 27 additions & 0 deletions jetty-client/src/main/java/org/eclipse/jetty/client/api/Request.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@

import org.eclipse.jetty.client.HttpClient;
import org.eclipse.jetty.client.util.InputStreamResponseListener;
import org.eclipse.jetty.http.HttpField;
import org.eclipse.jetty.http.HttpFields;
import org.eclipse.jetty.http.HttpHeader;
import org.eclipse.jetty.http.HttpMethod;
Expand Down Expand Up @@ -165,6 +166,32 @@ public interface Request
*/
HttpFields getHeaders();

/** Set the headers, clearing any existing headers
* @param fields The fields to set
* @return this request object
*/
Request set(HttpFields fields);

/**
* @param header the header to remove
* @return this request object
*/
Request remove(HttpHeader header);

/**
* @param field the field to add
* @return this request object
* @see #header(HttpHeader, String)
*/
Request add(HttpField field);

/**
* @param field the field to put
* @return this request object
* @see #header(HttpHeader, String)
*/
Request put(HttpField field);

/**
* @param name the name of the header
* @param value the value of the header
Expand Down
3 changes: 1 addition & 2 deletions jetty-client/src/main/java/org/eclipse/jetty/client/http/HttpSenderOverHTTP.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,7 @@ protected void sendHeaders(HttpExchange exchange, ByteBuffer contentBuffer, bool
String query = request.getQuery();
if (query != null)
path += "?" + query;
metaData = new MetaData.Request(request.getMethod(), new HttpURI(path), request.getVersion(), request.getHeaders(), contentLength);
metaData.setTrailerSupplier(request.getTrailers());
metaData = new MetaData.Request(request.getMethod(), HttpURI.from(path), request.getVersion(), request.getHeaders(), contentLength, request.getTrailers());
if (LOG.isDebugEnabled())
LOG.debug("Sending headers with content {} last={} for {}", BufferUtil.toDetailString(contentBuffer), lastContent, exchange.getRequest());
headersCallback.iterate();
Expand Down
2 changes: 1 addition & 1 deletion jetty-client/src/main/java/org/eclipse/jetty/client/util/BufferingResponseListener.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ public void onHeaders(Response response)

Request request = response.getRequest();
HttpFields headers = response.getHeaders();
long length = headers.getLongField(HttpHeader.CONTENT_LENGTH.asString());
long length = headers.getLongField(HttpHeader.CONTENT_LENGTH);
if (HttpMethod.HEAD.is(request.getMethod()))
length = 0;
if (length > maxLength)
Expand Down
5 changes: 4 additions & 1 deletion jetty-client/src/test/java/org/eclipse/jetty/client/ClientConnectionCloseTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,10 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques
HttpConnectionOverHTTP connection = (HttpConnectionOverHTTP)connectionPool.getActiveConnections().iterator().next();
assertFalse(connection.getEndPoint().isOutputShutdown());
})
.onResponseHeaders(r -> r.getHeaders().remove(HttpHeader.CONNECTION));
.onResponseHeaders(r ->
{
((HttpResponse)r).getHeaderFieldsMutable().remove(HttpHeader.CONNECTION);
});
ContentResponse response = request.send();

assertEquals(HttpStatus.OK_200, response.getStatus());
Expand Down
24 changes: 12 additions & 12 deletions jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientRedirectTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ public void test303(Scenario scenario) throws Exception
.send();
assertNotNull(response);
assertEquals(200, response.getStatus());
assertFalse(response.getHeaders().containsKey(HttpHeader.LOCATION.asString()));
assertFalse(response.getHeaders().contains(HttpHeader.LOCATION));
}

@ParameterizedTest
Expand All @@ -86,7 +86,7 @@ public void test303302(Scenario scenario) throws Exception
.send();
assertNotNull(response);
assertEquals(200, response.getStatus());
assertFalse(response.getHeaders().containsKey(HttpHeader.LOCATION.asString()));
assertFalse(response.getHeaders().contains(HttpHeader.LOCATION));
}

@ParameterizedTest
Expand All @@ -102,7 +102,7 @@ public void test303302OnDifferentDestinations(Scenario scenario) throws Exceptio
.send();
assertNotNull(response);
assertEquals(200, response.getStatus());
assertFalse(response.getHeaders().containsKey(HttpHeader.LOCATION.asString()));
assertFalse(response.getHeaders().contains(HttpHeader.LOCATION));
}

@ParameterizedTest
Expand All @@ -119,7 +119,7 @@ public void test301(Scenario scenario) throws Exception
.send();
assertNotNull(response);
assertEquals(200, response.getStatus());
assertFalse(response.getHeaders().containsKey(HttpHeader.LOCATION.asString()));
assertFalse(response.getHeaders().contains(HttpHeader.LOCATION));
}

@ParameterizedTest
Expand All @@ -139,7 +139,7 @@ public void test301WithWrongMethod(Scenario scenario) throws Exception
Response response = xx.getResponse();
assertNotNull(response);
assertEquals(301, response.getStatus());
assertTrue(response.getHeaders().containsKey(HttpHeader.LOCATION.asString()));
assertTrue(response.getHeaders().contains(HttpHeader.LOCATION));
}

@ParameterizedTest
Expand All @@ -158,7 +158,7 @@ public void test307WithRequestContent(Scenario scenario) throws Exception
.send();
assertNotNull(response);
assertEquals(200, response.getStatus());
assertFalse(response.getHeaders().containsKey(HttpHeader.LOCATION.asString()));
assertFalse(response.getHeaders().contains(HttpHeader.LOCATION));
assertArrayEquals(data, response.getContent());
}

Expand All @@ -179,7 +179,7 @@ public void testMaxRedirections(Scenario scenario) throws Exception
Response response = xx.getResponse();
assertNotNull(response);
assertEquals(302, response.getStatus());
assertTrue(response.getHeaders().containsKey(HttpHeader.LOCATION.asString()));
assertTrue(response.getHeaders().contains(HttpHeader.LOCATION));
}

@ParameterizedTest
Expand All @@ -195,7 +195,7 @@ public void test303WithConnectionCloseWithBigRequest(Scenario scenario) throws E
.send();
assertNotNull(response);
assertEquals(200, response.getStatus());
assertFalse(response.getHeaders().containsKey(HttpHeader.LOCATION.asString()));
assertFalse(response.getHeaders().contains(HttpHeader.LOCATION));
}

@ParameterizedTest
Expand All @@ -212,7 +212,7 @@ public void testDontFollowRedirects(Scenario scenario) throws Exception
.send();
assertNotNull(response);
assertEquals(303, response.getStatus());
assertTrue(response.getHeaders().containsKey(HttpHeader.LOCATION.asString()));
assertTrue(response.getHeaders().contains(HttpHeader.LOCATION));
}

@ParameterizedTest
Expand All @@ -228,7 +228,7 @@ public void testRelativeLocation(Scenario scenario) throws Exception
.send();
assertNotNull(response);
assertEquals(200, response.getStatus());
assertFalse(response.getHeaders().containsKey(HttpHeader.LOCATION.asString()));
assertFalse(response.getHeaders().contains(HttpHeader.LOCATION));
}

@ParameterizedTest
Expand All @@ -244,7 +244,7 @@ public void testAbsoluteURIPathWithSpaces(Scenario scenario) throws Exception
.send();
assertNotNull(response);
assertEquals(200, response.getStatus());
assertFalse(response.getHeaders().containsKey(HttpHeader.LOCATION.asString()));
assertFalse(response.getHeaders().contains(HttpHeader.LOCATION));
}

@ParameterizedTest
Expand All @@ -260,7 +260,7 @@ public void testRelativeURIPathWithSpaces(Scenario scenario) throws Exception
.send();
assertNotNull(response);
assertEquals(200, response.getStatus());
assertFalse(response.getHeaders().containsKey(HttpHeader.LOCATION.asString()));
assertFalse(response.getHeaders().contains(HttpHeader.LOCATION));
}

@ParameterizedTest
Expand Down
2 changes: 1 addition & 1 deletion jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -771,7 +771,7 @@ public void handle(String target, org.eclipse.jetty.server.Request baseRequest,

assertNotNull(response);
assertEquals(200, response.getStatus());
assertFalse(response.getHeaders().containsKey(headerName));
assertFalse(response.getHeaders().contains(headerName));
}

@ParameterizedTest
Expand Down
Loading

0 comments on commit 8c7e34f

Please sign in to comment.