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

Override HttpRequestDecoder.createMessage() for perfomance #4863

Merged
merged 29 commits into from
Apr 11, 2024
Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
1de569a
Fork netty's `httpServerCodec` and replace its usage
echo304 May 6, 2023
6b41e20
Fix test compile failure
echo304 May 7, 2023
ffa914f
Add licenses
echo304 May 9, 2023
9911367
Implement ArmeriaHttpHeaders
echo304 May 29, 2023
b688903
Implement `ArmeriaDefaultHttpRequest`
echo304 May 29, 2023
9de5522
Modify `HttpServerCodec.createMessage` to use `ArmeriaDefaultHttpRequ…
echo304 May 30, 2023
743a6fc
Modify `Http1RequestDecoder.channelRead` to use `ArmeriaDefaultHttpRe…
echo304 May 30, 2023
efd741e
Merge branch 'main' into custom-http-server-codec
echo304 Jun 2, 2023
bba7d8d
Reflect code review
echo304 Jun 3, 2023
8bccbc8
Fix styles
echo304 Jun 3, 2023
65ad9cd
Fix constructor of `ArmeriaHttpHeaders` to invoke `add()` method of i…
echo304 Jun 4, 2023
ce07dc1
Add `headers()` getter on `ArmeriaDefaultHttpRequest`
echo304 Jun 5, 2023
b81f037
Add test for `ArmeriaHttpHeaders` and fix logic
echo304 Jun 5, 2023
a2a2f48
Try another approach
echo304 Jun 8, 2023
deaa03c
Fix infinite loop bug
echo304 Jun 14, 2023
665c104
Embed building headers logic into `ArmeriaHttpHeaders.buildRequestHea…
echo304 Jun 21, 2023
2391c7b
Merge remote-tracking branch 'upstream/main' into custom-http-server-…
echo304 Jun 22, 2023
7515d8f
Fix ArmeriaHttpHeadersTest failed tests
echo304 Jul 5, 2023
89bc4ea
Merge branch 'main' into custom-http-server-codec
echo304 Jul 5, 2023
39092ea
Fix failed shouldSupportBindingOnDomainSocket test
echo304 Jul 5, 2023
c9f7520
Fix failed testUnsupportedMethod test
echo304 Jul 6, 2023
9ecd024
Fix styles
echo304 Jul 6, 2023
2cc3f13
Merge branch 'main' into custom-http-server-codec
echo304 Jul 11, 2023
f48f148
Merge branch 'main' into custom-http-server-codec
jrhee17 Mar 25, 2024
19ab386
minor cleanups
jrhee17 Mar 26, 2024
340a6f5
hide over-exposed class
jrhee17 Mar 26, 2024
ca51df6
lint
jrhee17 Mar 26, 2024
c80eb0c
Hide static methods and Change ArmeriaHttpHeadersFactory to enum
minwoox Apr 2, 2024
1eaad3a
minor fix
ikhoon Apr 9, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ public boolean equals(AsciiString a, AsciiString b) {
/**
* The set of headers that should not be directly copied when converting headers from HTTP/1 to HTTP/2.
*/
private static final CaseInsensitiveMap HTTP_TO_HTTP2_HEADER_DISALLOWED_LIST = new CaseInsensitiveMap();
public static final CaseInsensitiveMap HTTP_TO_HTTP2_HEADER_DISALLOWED_LIST = new CaseInsensitiveMap();

/**
* The set of headers that should not be directly copied when converting headers from HTTP/2 to HTTP/1.
Expand Down Expand Up @@ -236,8 +236,8 @@ public boolean equals(AsciiString a, AsciiString b) {
HttpHeaderNames.HOST);
}

private static final Splitter COOKIE_SPLITTER = Splitter.on(';').trimResults().omitEmptyStrings();
private static final String COOKIE_SEPARATOR = "; ";
public static final Splitter COOKIE_SPLITTER = Splitter.on(';').trimResults().omitEmptyStrings();
public static final String COOKIE_SEPARATOR = "; ";
private static final Joiner COOKIE_JOINER = Joiner.on(COOKIE_SEPARATOR);

@Nullable
Expand Down Expand Up @@ -729,7 +729,7 @@ public static void toArmeria(io.netty.handler.codec.http.HttpHeaders inHeaders,
}
}

private static boolean maybeWebSocketUpgrade(AsciiString header, CharSequence value) {
public static boolean maybeWebSocketUpgrade(AsciiString header, CharSequence value) {
if (HttpHeaderNames.CONNECTION.contentEqualsIgnoreCase(header) &&
HttpHeaderValues.UPGRADE.contentEqualsIgnoreCase(value)) {
return true;
Expand All @@ -738,43 +738,49 @@ private static boolean maybeWebSocketUpgrade(AsciiString header, CharSequence va
HttpHeaderValues.WEBSOCKET.contentEqualsIgnoreCase(value);
}

private static CaseInsensitiveMap toLowercaseMap(Iterator<? extends CharSequence> valuesIter,
int arraySizeHint) {
final CaseInsensitiveMap result = new CaseInsensitiveMap(arraySizeHint);
public static CaseInsensitiveMap toLowercaseMap(Iterator<? extends CharSequence> valuesIter,
int arraySizeHint) {
final CaseInsensitiveMap resultMap = new CaseInsensitiveMap(arraySizeHint);

while (valuesIter.hasNext()) {
final AsciiString lowerCased = AsciiString.of(valuesIter.next()).toLowerCase();
try {
int index = lowerCased.forEachByte(FIND_COMMA);
if (index != -1) {
int start = 0;
do {
result.add(lowerCased.subSequence(start, index, false).trim(), EMPTY_STRING);
start = index + 1;
} while (start < lowerCased.length() &&
(index = lowerCased.forEachByte(start,
lowerCased.length() - start, FIND_COMMA)) != -1);
result.add(lowerCased.subSequence(start, lowerCased.length(), false).trim(), EMPTY_STRING);
} else {
result.add(lowerCased.trim(), EMPTY_STRING);
}
} catch (Exception e) {
// This is not expect to happen because FIND_COMMA never throws but must be caught
// because of the ByteProcessor interface.
throw new IllegalStateException(e);
splitByCommaAndAdd(resultMap, lowerCased);
}
return resultMap;
}

public static void splitByCommaAndAdd(CaseInsensitiveMap result, AsciiString lowerCased) {
try {
int index = lowerCased.forEachByte(FIND_COMMA);
if (index != -1) {
int start = 0;
do {
result.add(lowerCased.subSequence(start, index, false).trim(), EMPTY_STRING);
start = index + 1;
} while (start < lowerCased.length() &&
(index = lowerCased.forEachByte(start,
lowerCased.length() - start,
FIND_COMMA)) != -1);
result.add(lowerCased.subSequence(start, lowerCased.length(), false).trim(),
EMPTY_STRING);
} else {
result.add(lowerCased.trim(), EMPTY_STRING);
}
} catch (Exception e) {
// This is not expect to happen because FIND_COMMA never throws but must be caught
// because of the ByteProcessor interface.
throw new IllegalStateException(e);
}
return result;
}

/**
* Filter the {@link HttpHeaderNames#TE} header according to the
* <a href="https://datatracker.ietf.org/doc/html/rfc7540#section-8.1.2.2">special rules in the HTTP/2 RFC</a>.
*
* @param entry the entry whose name is {@link HttpHeaderNames#TE}.
* @param out the resulting HTTP/2 headers.
*/
private static void toHttp2HeadersFilterTE(Entry<CharSequence, CharSequence> entry,
/**
* Filter the {@link HttpHeaderNames#TE} header according to the
* <a href="https://datatracker.ietf.org/doc/html/rfc7540#section-8.1.2.2">special rules in the HTTP/2 RFC</a>.
*
* @param entry the entry whose name is {@link HttpHeaderNames#TE}.
* @param out the resulting HTTP/2 headers.
*/
public static void toHttp2HeadersFilterTE(Entry<CharSequence, CharSequence> entry,
HttpHeadersBuilder out) {
if (AsciiString.indexOf(entry.getValue(), ',', 0) == -1) {
if (AsciiString.contentEqualsIgnoreCase(AsciiString.trim(entry.getValue()),
Expand Down Expand Up @@ -1075,7 +1081,7 @@ public static boolean isTrailerDisallowed(AsciiString name) {
return HTTP_TRAILER_DISALLOWED_LIST.contains(name);
}

private static final class CaseInsensitiveMap
public static final class CaseInsensitiveMap
extends DefaultHeaders<AsciiString, AsciiString, CaseInsensitiveMap> {

CaseInsensitiveMap() {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,147 @@
/*
* Copyright 2023 LINE Corporation
*
* LINE Corporation licenses this file to you under the Apache License,
* version 2.0 (the "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at:
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations
* under the License.
*/
package com.linecorp.armeria.server;
ikhoon marked this conversation as resolved.
Show resolved Hide resolved

import static com.google.common.base.Preconditions.checkNotNull;

import java.util.Map;

import com.google.common.base.MoreObjects;
import com.google.common.base.MoreObjects.ToStringHelper;

import com.linecorp.armeria.common.RequestHeaders;
import com.linecorp.armeria.common.RequestHeadersBuilder;

import io.netty.handler.codec.http.DefaultHttpMessage;
import io.netty.handler.codec.http.HttpHeaders;
import io.netty.handler.codec.http.HttpMethod;
import io.netty.handler.codec.http.HttpRequest;
import io.netty.handler.codec.http.HttpVersion;
import io.netty.util.internal.ObjectUtil;

final class ArmeriaDefaultHttpRequest extends DefaultHttpMessage implements HttpRequest {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a specific reason to extend DefaultHttpMessage? That internally creates DefaultHttpHeaders again. Duplicate headers may result in performance degression.

Suggested change
final class ArmeriaDefaultHttpRequest extends DefaultHttpMessage implements HttpRequest {
final class ArmeriaHttp1Request implements HttpRequest {

private static final int HASH_CODE_PRIME = 31;
private HttpMethod method;
private final RequestHeadersBuilder builder;
private final HttpHeaders headers;
private String uri;

/**
* Creates a new instance.
*
* @param httpVersion the HTTP version of the request
* @param method the HTTP method of the request
* @param uri the URI or path of the request
* @param validateHeaders validate the header names and values when adding them to the {@link HttpHeaders}
*/
ArmeriaDefaultHttpRequest(HttpVersion httpVersion, HttpMethod method, String uri,
boolean validateHeaders) {
super(httpVersion, validateHeaders, false);
this.method = checkNotNull(method, "method");
this.uri = checkNotNull(uri, "uri");
final com.linecorp.armeria.common.HttpMethod armeriaMethod =
com.linecorp.armeria.common.HttpMethod.tryParse(method.name());
builder = RequestHeaders.builder(armeriaMethod != null ? armeriaMethod
: com.linecorp.armeria.common.HttpMethod.UNKNOWN,
uri);
headers = new ArmeriaHttpHeaders(builder);
}

@Override
public HttpHeaders headers() {
return headers;
}

@Override
@Deprecated
public HttpMethod getMethod() {
return method();
}

@Override
public HttpMethod method() {
return method;
}

@Override
@Deprecated
public String getUri() {
return uri();
}

@Override
public String uri() {
return uri;
}

public RequestHeadersBuilder requestHeadersBuilder() {
return builder;
}

@Override
public HttpRequest setMethod(HttpMethod method) {
this.method = ObjectUtil.checkNotNull(method, "method");
return this;
}

@Override
public HttpRequest setUri(String uri) {
this.uri = ObjectUtil.checkNotNull(uri, "uri");
return this;
}

@Override
public HttpRequest setProtocolVersion(HttpVersion version) {
super.setProtocolVersion(version);
return this;
}

@Override
public int hashCode() {
int result = 1;
result = HASH_CODE_PRIME * result + method.hashCode();
result = HASH_CODE_PRIME * result + uri.hashCode();
result = HASH_CODE_PRIME * result + super.hashCode();
return result;
}

@Override
public boolean equals(Object o) {
if (!(o instanceof ArmeriaDefaultHttpRequest)) {
return false;
}

final ArmeriaDefaultHttpRequest other = (ArmeriaDefaultHttpRequest) o;

return method().equals(other.method()) &&
uri().equalsIgnoreCase(other.uri()) &&
super.equals(o);
}

@Override
public String toString() {
final ToStringHelper toStringHelper = MoreObjects.toStringHelper(this)
.add("decoderResult", decoderResult())
.add("version", protocolVersion())
.add("method", method());

for (Map.Entry<String, String> e: headers()) {
toStringHelper.add(e.getKey(), e.getValue());
}

return toStringHelper.toString();
}
}