ensure the XContentBuilder is always closed in RestBuilderListener #21124
Conversation
There may be cases where the XContentBuilder is not used and therefore it never gets closed, which can cause a leak of bytes. This change moves the creation of the builder into a try with resources block.
jaymode
added
>bug
:Core/Infra/REST API
|
I think this should go as well in 5.0 branch, no? |
|
I'm not sure we want to automatically close. The current api is set up so that we expect to use the builder we give them (there is no way to not have a builder) - if they don't it's probably a bug we're hiding (or we should give an official way to not have a builder created at all). I think we should add an assertion that verifies the builder is closed when buildResponse returns |
|
@bleskes maybe both, assertion and auto close? I mean some plugins might not test this (3rd party ) and then we are leaking bytes since assertions are off? |
|
@s1monw yeah , that's what I meant ++ . Keep code as is but make sure builder is closed when buildResponse returns |
I think that makes sense, I added the 5.0.1 label.
I pushed 5a17d14, which adds the assertion while keeping auto close. |
| return buildResponse(response, builder); | ||
| try (XContentBuilder builder = channel.newBuilder()) { | ||
| final RestResponse restResponse = buildResponse(response, builder); | ||
| assert assertionsEnabled == false || builder.isClosed() : "callers should ensure the XContentBuilder is closed themselves"; |
There was a problem hiding this comment.
can you add a protected method assert assertBuilderClosed(builder) we can override in tests instead. this is very confusing and I hate static mutable vars
There was a problem hiding this comment.
++ that's a much better approach
| public boolean isClosed() { | ||
| return generator.isClosed(); | ||
| } | ||
|
|
||
| public XContentGenerator generator() { |
There was a problem hiding this comment.
would it be enough to only add isClosed to XContentGenerator? and maybe add a javadoc?
|
|
||
| // pkg private method that we can override for testing | ||
| boolean assertBuilderClosed(XContentBuilder xContentBuilder) { | ||
| return xContentBuilder.generator().isClosed(); |
There was a problem hiding this comment.
I usually do this:
assert xContentBuilder.generator().isClosed();
return true;
|
left a minor LGTM otherwise |
|
@jaymode can we get this in please? |
There may be cases where the XContentBuilder is not used and therefore it never gets closed, which can cause a leak of bytes. This change moves the creation of the builder into a try with resources block.