Skip to content

Commit

Permalink
Return immutable headers
Browse files Browse the repository at this point in the history
  • Loading branch information
@minwoox
minwoox committed Jan 30, 2019
1 parent 44a9666 commit 02e39a0
Show file tree
Hide file tree
Showing 6 changed files with 45 additions and 42 deletions.
4 changes: 2 additions & 2 deletions core/src/main/java/com/linecorp/armeria/common/AggregatedHttpMessage.java
View file
Expand Up @@ -18,7 +18,7 @@

import static com.linecorp.armeria.common.HttpHeaderNames.CONTENT_LENGTH;
import static com.linecorp.armeria.internal.ArmeriaHttpUtil.isContentAlwaysEmpty;
import static com.linecorp.armeria.internal.ArmeriaHttpUtil.setOrRemoveContentLengthInResponseHeaders;
import static com.linecorp.armeria.internal.ArmeriaHttpUtil.setOrRemoveContentLength;
import static java.util.Objects.requireNonNull;

import java.nio.charset.StandardCharsets;
Expand Down Expand Up @@ -340,7 +340,7 @@ static AggregatedHttpMessage of(Iterable<HttpHeaders> informationals, HttpHeader
final HttpStatus status = headers.status();
final HttpHeaders newHeaders;
if (status != null) { // Response
newHeaders = setOrRemoveContentLengthInResponseHeaders(headers,content, trailingHeaders);
newHeaders = setOrRemoveContentLength(headers, content, trailingHeaders);
} else { // Request
newHeaders = headers.toMutable();
if (content.isEmpty()) {
Expand Down
5 changes: 2 additions & 3 deletions core/src/main/java/com/linecorp/armeria/common/HttpResponse.java
View file
Expand Up @@ -17,7 +17,7 @@
package com.linecorp.armeria.common;

import static com.linecorp.armeria.internal.ArmeriaHttpUtil.isContentAlwaysEmpty;
import static com.linecorp.armeria.internal.ArmeriaHttpUtil.setOrRemoveContentLengthInResponseHeaders;
import static com.linecorp.armeria.internal.ArmeriaHttpUtil.setOrRemoveContentLength;
import static java.util.Objects.requireNonNull;

import java.nio.charset.StandardCharsets;
Expand Down Expand Up @@ -252,8 +252,7 @@ static HttpResponse of(HttpHeaders headers, HttpData content, HttpHeaders traili
throw new IllegalStateException("not a response (missing :status)");
}

final HttpHeaders newHeaders = setOrRemoveContentLengthInResponseHeaders(headers, content,
trailingHeaders);
final HttpHeaders newHeaders = setOrRemoveContentLength(headers, content, trailingHeaders);
if (content.isEmpty() && trailingHeaders.isEmpty()) {
ReferenceCountUtil.safeRelease(content);
return new OneElementFixedHttpResponse(newHeaders);
Expand Down
17 changes: 10 additions & 7 deletions core/src/main/java/com/linecorp/armeria/internal/ArmeriaHttpUtil.java
View file
Expand Up @@ -739,8 +739,8 @@ public static void toNettyHttp1(
* @throws IllegalArgumentException if the specified {@code content} or {@code trailingHeaders} are
* non-empty when the content is always empty
*/
public static HttpHeaders setOrRemoveContentLengthInResponseHeaders(HttpHeaders headers, HttpData content,
HttpHeaders trailingHeaders) {
public static HttpHeaders setOrRemoveContentLength(HttpHeaders headers, HttpData content,
HttpHeaders trailingHeaders) {
requireNonNull(headers, "headers");
requireNonNull(content, "content");
requireNonNull(trailingHeaders, "trailingHeaders");
Expand All @@ -753,22 +753,25 @@ public static HttpHeaders setOrRemoveContentLengthInResponseHeaders(HttpHeaders
if (status != HttpStatus.NOT_MODIFIED) {
mutable.remove(HttpHeaderNames.CONTENT_LENGTH);
} else {
// 304 response can have the 'Content-length' header when it is a response to a conditional
// 304 response can have the "content-length" header when it is a response to a conditional
// GET request. See https://tools.ietf.org/html/rfc7230#section-3.3.2
}
} else if (!trailingHeaders.isEmpty()) {
// Some of the client implementations such as "curl" ignores trailing headers if
// the "Content-length" header is present. We should not set Content-length header when trailing
// the "content-length" header is present. We should not set "content-length" header when trailing
// headers exists so that those clients can receive the trailing headers.
// The response is sent using chunked transfer encoding in HTTP/1 and in DATA frame payload
// in HTTP/2, so it's no worry.
mutable.remove(HttpHeaderNames.CONTENT_LENGTH);
} else if (!headers.contains(HttpHeaderNames.CONTENT_LENGTH)) {
} else if (!headers.contains(HttpHeaderNames.CONTENT_LENGTH) || !content.isEmpty()) {
mutable.setInt(HttpHeaderNames.CONTENT_LENGTH, content.length());
} else {
// Do not overwrite the 'content-length' header because a response to a HEAD request
// The header contains "content-length" header and the content is empty.
// Do not overwrite the header because a response to a HEAD request
// will have no content even if it has non-zero content-length header.
}

return mutable;
return mutable.asImmutable();
}

private static String convertHeaderValue(AsciiString name, CharSequence value) {
Expand Down
46 changes: 24 additions & 22 deletions core/src/main/java/com/linecorp/armeria/server/HttpServerHandler.java
View file
Expand Up @@ -575,42 +575,46 @@ private void respond(ChannelHandlerContext ctx, DecodedHttpRequest req, HttpStat
private void respond(ChannelHandlerContext ctx, DecodedHttpRequest req, AggregatedHttpMessage res,
RequestContext reqCtx, @Nullable Throwable cause) {
if (!handledLastRequest) {
addKeepAliveHeaders(req, res);
respond0(ctx, req, res, reqCtx, cause).addListener(CLOSE_ON_FAILURE);
respond0(req, res, reqCtx, true, cause).addListener(CLOSE_ON_FAILURE);
} else {
// Note that it is perfectly fine not to set the 'content-length' header to the last response
// of an HTTP/1 connection. We set it anyway to work around overly strict HTTP clients that always
// require a 'content-length' header for non-chunked responses.
setContentLength(req, res);
respond0(ctx, req, res, reqCtx, cause).addListener(CLOSE);
respond0(req, res, reqCtx, false, cause).addListener(CLOSE);
}

if (!isReading) {
ctx.flush();
}
}

private ChannelFuture respond0(ChannelHandlerContext ctx,
DecodedHttpRequest req, AggregatedHttpMessage res,
RequestContext reqCtx, @Nullable Throwable cause) {
private ChannelFuture respond0(DecodedHttpRequest req, AggregatedHttpMessage res, RequestContext reqCtx,
boolean addKeepAlive, @Nullable Throwable cause) {

// No need to consume further since the response is ready.
req.close();

final boolean trailingHeadersEmpty = res.trailingHeaders().isEmpty();
final boolean contentAndTrailingHeadersEmpty = res.content().isEmpty() && trailingHeadersEmpty;
final HttpData content = res.content();
final boolean contentAndTrailingHeadersEmpty = content.isEmpty() && trailingHeadersEmpty;

final RequestLogBuilder logBuilder = reqCtx.logBuilder();

logBuilder.startResponse();
assert responseEncoder != null;
final HttpHeaders mutableHeaders = res.headers().toMutable();
if (addKeepAlive) {
addKeepAliveHeaders(req, mutableHeaders);
}
// Note that it is perfectly fine not to set the 'content-length' header to the last response
// of an HTTP/1 connection. We set it anyway to work around overly strict HTTP clients that always
// require a 'content-length' header for non-chunked responses.
setContentLength(req, mutableHeaders, content.length());

ChannelFuture future = responseEncoder.writeHeaders(
req.id(), req.streamId(), res.headers(), contentAndTrailingHeadersEmpty);
logBuilder.responseHeaders(res.headers());
req.id(), req.streamId(), mutableHeaders, contentAndTrailingHeadersEmpty);
logBuilder.responseHeaders(mutableHeaders);
if (!contentAndTrailingHeadersEmpty) {
future = responseEncoder.writeData(
req.id(), req.streamId(), res.content(), trailingHeadersEmpty);
logBuilder.increaseResponseLength(res.content().length());
req.id(), req.streamId(), content, trailingHeadersEmpty);
logBuilder.increaseResponseLength(content.length());
if (!trailingHeadersEmpty) {
future = responseEncoder.writeHeaders(
req.id(), req.streamId(), res.trailingHeaders(), true);
Expand All @@ -633,28 +637,26 @@ private ChannelFuture respond0(ChannelHandlerContext ctx,
* Sets the keep alive header as per:
* - https://www.w3.org/Protocols/HTTP/1.1/draft-ietf-http-v11-spec-01.html#Connection
*/
private void addKeepAliveHeaders(HttpRequest req, AggregatedHttpMessage res) {
private void addKeepAliveHeaders(HttpRequest req, HttpHeaders headers) {
if (protocol == H1 || protocol == H1C) {
res.headers().set(HttpHeaderNames.CONNECTION, "keep-alive");
headers.set(HttpHeaderNames.CONNECTION, "keep-alive");
} else {
// Do not add the 'connection' header for HTTP/2 responses.
// See https://tools.ietf.org/html/rfc7540#section-8.1.2.2
}

setContentLength(req, res);
}

/**
* Sets the 'content-length' header to the response.
*/
private static void setContentLength(HttpRequest req, AggregatedHttpMessage res) {
private static void setContentLength(HttpRequest req, HttpHeaders headers, int contentLength) {
// https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.4
// prohibits to send message body for below cases.
// and in those cases, content should be empty.
if (req.method() == HttpMethod.HEAD || ArmeriaHttpUtil.isContentAlwaysEmpty(res.status())) {
if (req.method() == HttpMethod.HEAD || ArmeriaHttpUtil.isContentAlwaysEmpty(headers.status())) {
return;
}
res.headers().setInt(HttpHeaderNames.CONTENT_LENGTH, res.content().length());
headers.setInt(HttpHeaderNames.CONTENT_LENGTH, contentLength);
}

@Nullable
Expand Down
11 changes: 6 additions & 5 deletions core/src/main/java/com/linecorp/armeria/server/encoding/HttpEncodedResponse.java
View file
Expand Up @@ -97,18 +97,19 @@ protected HttpObject filter(HttpObject obj) {
encodedStream = new ByteArrayOutputStream();
encodingStream = HttpEncoders.getEncodingOutputStream(encodingType, encodedStream);

final HttpHeaders mutable = headers.toMutable();
// Always use chunked encoding when compressing.
headers.remove(HttpHeaderNames.CONTENT_LENGTH);
mutable.remove(HttpHeaderNames.CONTENT_LENGTH);
switch (encodingType) {
case GZIP:
headers.set(HttpHeaderNames.CONTENT_ENCODING, "gzip");
mutable.set(HttpHeaderNames.CONTENT_ENCODING, "gzip");
break;
case DEFLATE:
headers.set(HttpHeaderNames.CONTENT_ENCODING, "deflate");
mutable.set(HttpHeaderNames.CONTENT_ENCODING, "deflate");
break;
}
headers.set(HttpHeaderNames.VARY, HttpHeaderNames.ACCEPT_ENCODING.toString());
return headers;
mutable.set(HttpHeaderNames.VARY, HttpHeaderNames.ACCEPT_ENCODING.toString());
return mutable;
}

if (encodingStream == null) {
Expand Down
4 changes: 1 addition & 3 deletions core/src/test/java/com/linecorp/armeria/common/DefaultAggregatedHttpMessageTest.java
View file
Expand Up @@ -213,9 +213,7 @@ public void contentLengthIsSet() {
assertThat(AggregatedHttpMessage.of(headers).headers().getInt(CONTENT_LENGTH)).isEqualTo(1000000);

msg = AggregatedHttpMessage.of(headers, HttpData.ofUtf8("foo"));
// TODO(minwoox) The length is different from the content length because the user specified it with
// the wrong value. Should we prevent this?
assertThat(msg.headers().getInt(CONTENT_LENGTH)).isEqualTo(1000000);
assertThat(msg.headers().getInt(CONTENT_LENGTH)).isEqualTo(3);
}

@Test
Expand Down

0 comments on commit 02e39a0

Please sign in to comment.