Fixed ByteBuf leaking in org.elasticsearch.http.netty4.Netty4HttpRequestHandler #27222
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…pRequestHandler#channelRead0
original-brownbear
changed the title
jasontedor
requested changes
Nov 2, 2017
| try { | ||
| httpRequest = new Netty4HttpRequest(serverTransport.xContentRegistry, copy, ctx.channel()); | ||
| } catch (Exception ex) { | ||
| request.release(); |
There was a problem hiding this comment.
I don't think this is right, I think this should only be released here if we added a retain which only happens if msg is an instance of HttpPipelinedRequest.
|
@jasontedor thanks, my mistake. Made it only fire the |
jasontedor
added
:Distributed Coordination/Network
|
Thanks @original-brownbear, this looks good to me now. I will pull this in after CI completes. |
|
@jasontedor thanks for the review :) Failure looks unrelated? 14:23:17 Executing task ':checkout-6.x:core:compileJava' (up-to-date check took 0.485 secs) due to:
14:23:17 No history is available.
14:23:17 All input files are considered out-of-date for incremental task ':checkout-6.x:core:compileJava'.
14:23:17 :core:compileJava - is not incremental (e.g. outputs have changed, no previous execution, etc.).
14:23:17 Compiling with Java command line compiler '/usr/lib/jvm/java-8-openjdk-amd64/bin/javac'.
14:23:18 Starting process 'command '/usr/lib/jvm/java-8-openjdk-amd64/bin/javac''. Working directory: /var/lib/jenkins/workspace/elastic+elasticsearch+pull-request/distribution/bwc/build/bwc/checkout-6.x/core Command: /usr/lib/jvm/java-8-openjdk-amd64/bin/javac -J-Xmx1g @/var/lib/jenkins/workspace/elastic+elasticsearch+pull-request/distribution/bwc/build/bwc/checkout-6.x/core/build/tmp/compileJava/java-compiler-args.txt
14:23:18 Successfully started process 'command '/usr/lib/jvm/java-8-openjdk-amd64/bin/javac''
14:23:18 Picked up JAVA_TOOL_OPTIONS: -Dfile.encoding=UTF8
14:23:37 /var/lib/jenkins/workspace/elastic+elasticsearch+pull-request/distribution/bwc/build/bwc/checkout-6.x/core/src/main/java/org/elasticsearch/search/SearchService.java:652: error: cannot find symbol
14:23:37 throw new QueryPhaseExecutionException(context,
14:23:37 ^
14:23:37 symbol: class QueryPhaseExecutionException
14:23:37 location: class SearchService
14:23:44 Note: Some input files use or override a deprecated API.
14:23:44 Note: Recompile with -Xlint:deprecation for details.
14:23:44 Note: Some input files use unchecked or unsafe operations.
14:23:44 Note: Recompile with -Xlint:unchecked for details.
14:23:44 1 error
14:23:44 :checkout-6.x:core:compileJava FAILED |
|
I guess it was related to 081e7e3 |
* master: Remove unused searcher parameter in SearchService#createContext (elastic#27227) Upgrade to Lucene 7.1 (elastic#27225) Move IndexShard#getWritingBytes() under InternalEngine (elastic#27209) Adjust bwc version for exists query tests
jasontedor
pushed a commit
that referenced
this pull request
Nov 3, 2017
If creating the REST request throws an exception (for example, because of invalid headers), we leak the request due to failure to release the buffer (which would otherwise happen after replying on the channel). This commit addresses this leak by handling the failure case. Relates #27222
jasontedor
pushed a commit
that referenced
this pull request
Nov 3, 2017
If creating the REST request throws an exception (for example, because of invalid headers), we leak the request due to failure to release the buffer (which would otherwise happen after replying on the channel). This commit addresses this leak by handling the failure case. Relates #27222
jasontedor
pushed a commit
that referenced
this pull request
Nov 3, 2017
If creating the REST request throws an exception (for example, because of invalid headers), we leak the request due to failure to release the buffer (which would otherwise happen after replying on the channel). This commit addresses this leak by handling the failure case. Relates #27222
|
Thanks again @original-brownbear. |
clintongormley
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I found a spot where we were leaking
ByteBufhere:You can reproduce the leak by sending a request with broken HTTP headers like e.g.
(note duplicate
Content-Type)This results in the following exception being thrown:
as well as this leak being logged:
What's causing this, is that the
msgreceived by thischannelRead0has arefCntof2and the finally block in the parentchannelReadonly releases once. The secondreleasehappens down-stream in:which is not executed if we throw on invalid headers in the constructor of
Netty4HttpRequest.I'm not sure if the current behavior is actually desirable? Do we actually want to just close the client connection here with an empty response if we receive broken headers?
That said if this is the behavior we're going for I think this PR is a valid fix, by simply handling the missing
releasein case of an exception.