-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
[BUG] XContentBuilder.toString() will close the builder itself #13731
Comments
@chishui Thanks for reporting this! I think it is a pretty common with the builder pattern that a builder isn't necessarily mutable after "building" it. It's a fair question as to whether toString-ing the builder should implicitly "build" it. I don't know exactly the specifics here, but in order to avoid this problem you may well have to make a deep copy of the builder state when toString-ing it, which would have performance implications. Would you care to put up a PR attempting to fix this? I would also probably be okay with just explicitly documenting the toString() method as a terminal operation on the builder that closes it. The |
Many implementations neither explicitly call |
Describe the bug
We are using XContentBuilder in ml-commons plugin https://github.com/opensearch-project/ml-commons/blob/0483d14f1ce39af546633ee460e0ec7afa54ccbd/common/src/main/java/org/opensearch/ml/common/connector/functions/preprocess/DefaultPreProcessFunction.java#L45.
When I debug code linked above, I found
mlInput.toXContent(builder, EMPTY_PARAMS);
would throw an NPE exceptionHowever, if I set breakpoint not right at
mlInput.toXContent(builder, EMPTY_PARAMS);
but the line below, there is no NPE.With help of @zane-neo, I realized that XContentBuilder.toString() will call
BytesReference.bytes(this)
and BytesReference will close the XContentBuilder before making bytes.The issue I saw was because the debugger would call XContentBuilder.toString() which close the instance which was very unexpected. This seems to be a very weird behavior and could cause some serious issue when people accidentally call toString(). And since XContentBuilder already extends Closeable, it's a contract that people should call
close
explicitly or put builder in a try clause, we shouldn't usetoString()
as an implicit signal to close XContentBuilder.Related component
Libraries
To Reproduce
or
Expected behavior
Implicit or explicit call of toString() of
XContentBuilder
should not unexpected close it.Additional Details
Plugins
The issue was found in ml-commons plugin, but it's a general issue.
Screenshots
If applicable, add screenshots to help explain your problem.
Host/Environment (please complete the following information):
Additional context
Add any other context about the problem here.
The text was updated successfully, but these errors were encountered: