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.

Sign up for GitHub

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

Jump to bottom

Wrap stream passed to createParser in try-with-resources #28897

Merged
merged 2 commits into from
Mar 4, 2018
Merged

Wrap stream passed to createParser in try-with-resources #28897

merged 2 commits into from
Mar 4, 2018

Conversation

dakrone
Copy link
Member

@dakrone dakrone commented Mar 4, 2018

This wraps the stream (.streamInput()) that is passed to many of the
createParser instances in the enclosing (or a new) try-with-resources block.
This ensures the BytesReference.streamInput() is closed.

Relates to #28504

@dakrone
Wrap stream passed to createParser in try-with-resources
ec1a3be 
This wraps the stream (`.streamInput()`) that is passed to many of the
`createParser` instances in the enclosing (or a new) try-with-resources block.
This ensures the `BytesReference.streamInput()` is closed.

Relates to elastic#28504
@dakrone dakrone requested a review from nik9000 March 4, 2018 19:16
s1monw
s1monw approved these changes Mar 4, 2018
View reviewed changes
Copy link
Contributor

@s1monw s1monw left a comment

Choose a reason for hiding this comment

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

LGTM

server/src/main/java/org/elasticsearch/common/xcontent/XContentHelper.java Outdated
@@ -106,7 +106,11 @@ public static XContentParser createParser(NamedXContentRegistry xContentRegistry
input = bytes.streamInput();
}
contentType = xContentType != null ? xContentType : XContentFactory.xContentType(input);
return new Tuple<>(Objects.requireNonNull(contentType), convertToMap(XContentFactory.xContent(contentType), input, ordered));
try {
Copy link
Contributor

Choose a reason for hiding this comment

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

why does try/with not work here?

Copy link
Member Author

Choose a reason for hiding this comment

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

It should, I just need to re-assign the input stream to a different var so that var can be closed. I'll do that

@dakrone dakrone merged commit e7d1e12 into elastic:master Mar 4, 2018
dakrone added a commit that referenced this pull request Mar 5, 2018
@dakrone
Wrap stream passed to createParser in try-with-resources (#28897)
8f91a6a 
* Wrap stream passed to createParser in try-with-resources

This wraps the stream (`.streamInput()`) that is passed to many of the
`createParser` instances in the enclosing (or a new) try-with-resources block.
This ensures the `BytesReference.streamInput()` is closed.

Relates to #28504

* Use try-with-resources instead of closing in a finally block
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Mar 7, 2018
@jasontedor
Merge branch 'master' into unknown-or-invalid-settings-updates
fe756a3 
* master:
  [TEST] AwaitsFix QueryRescorerIT.testRescoreAfterCollapse
  Decouple XContentType from StreamInput/Output (elastic#28927)
  Remove BytesRef usage from XContentParser and its subclasses (elastic#28792)
  [DOCS] Correct typo in configuration (elastic#28903)
  Fix incorrect datemath example (elastic#28904)
  Add a usage example of the JLH score (elastic#28905)
  Wrap stream passed to createParser in try-with-resources (elastic#28897)
  Rescore collapsed documents (elastic#28521)
  Fix (simple)_query_string to ignore removed terms (elastic#28871)
  [Docs] Fix typo in composite aggregation (elastic#28891)
  Try if tombstone is eligable for pruning before locking on it's key (elastic#28767)
martijnvg added a commit that referenced this pull request Mar 8, 2018
@martijnvg
Merge remote-tracking branch 'es/master' into ccr
bfd587d 
* es/master: (48 commits)
  Update bucket-sort-aggregation.asciidoc (#28937)
  [Docs] REST high-level client: Fix code for most basic search request (#28916)
  Improved percolator's random candidate query duel test and fixed bugs that were exposed by this:
  Revert "Rescore collapsed documents (#28521)"
  Build: Fix test logger NPE when no tests are run (#28929)
  [TEST] AwaitsFix QueryRescorerIT.testRescoreAfterCollapse
  Decouple XContentType from StreamInput/Output (#28927)
  Remove BytesRef usage from XContentParser and its subclasses (#28792)
  [DOCS] Correct typo in configuration (#28903)
  Fix incorrect datemath example (#28904)
  Add a usage example of the JLH score (#28905)
  Wrap stream passed to createParser in try-with-resources (#28897)
  Rescore collapsed documents (#28521)
  Fix (simple)_query_string to ignore removed terms (#28871)
  [Docs] Fix typo in composite aggregation (#28891)
  Try if tombstone is eligable for pruning before locking on it's key (#28767)
  Limit analyzed text for highlighting (improvements) (#28808)
  Missing `timeout` parameter from the REST API spec JSON files (#28328)
  Clarifies how query_string splits textual part (#28798)
  Update outdated java version reference (#28870)
  ...
martijnvg added a commit that referenced this pull request Mar 8, 2018
@martijnvg
Merge remote-tracking branch 'es/6.x' into ccr-6.x
144882b 
* es/6.x: (48 commits)
  Update bucket-sort-aggregation.asciidoc (#28937)
  [Docs] REST high-level client: Fix code for most basic search request (#28916)
  Improved percolator's random candidate query duel test and fixed bugs that were exposed by this:
  Revert "Rescore collapsed documents (#28521)"
  Build: Fix test logger NPE when no tests are run (#28929)
  [TEST] AwaitsFix QueryRescorerIT.testRescoreAfterCollapse
  Decouple XContentType from StreamInput/Output (#28927)
  Remove BytesRef usage from XContentParser and its subclasses (#28792)
  Add doc note for -server flag on Windows service
  [DOCS] Correct typo in configuration (#28903)
  Fix incorrect datemath example (#28904)
  Add a usage example of the JLH score (#28905)
  Limit analyzed text for highlighting (improvements) (#28907)
  Wrap stream passed to createParser in try-with-resources (#28897)
  [Docs] Fix typo in composite aggregation (#28891)
  Rescore collapsed documents (#28521)
  Fix (simple)_query_string to ignore removed terms (#28871)
  Missing `timeout` parameter from the REST API spec JSON files (#28328)
  Clarifies how query_string splits textual part (#28798)
  Update outdated java version reference (#28870)
  ...
sebasjm pushed a commit to sebasjm/elasticsearch that referenced this pull request Mar 10, 2018
@dakrone
Wrap stream passed to createParser in try-with-resources (elastic#28897)
00d7a79 
* Wrap stream passed to createParser in try-with-resources

This wraps the stream (`.streamInput()`) that is passed to many of the
`createParser` instances in the enclosing (or a new) try-with-resources block.
This ensures the `BytesReference.streamInput()` is closed.

Relates to elastic#28504

* Use try-with-resources instead of closing in a finally block
@dakrone dakrone deleted the close-parser-streams branch April 19, 2018 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@s1monw s1monw s1monw approved these changes

@nik9000 nik9000 Awaiting requested review from nik9000

Assignees
No one assigned
Labels
>non-issue v6.3.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@dakrone @s1monw @colings86