Avoid leaking memory in SseParser

[geoand]

Relates to: #23997 (comment)

[geoand]

Tests pass. @FroMage do you agree with this one?

[FroMage]

Well, it really depends on the semantics of the event, if the buffer is reused for every event or not. If it's reused, then we shouldn't release it. If the consumers have to release it, then sure.

[geoand]

Where would it be reused?

[FroMage]

By the code sending us notifications of new data to read.

[FroMage]

Docs say:

  /**
   * Set a data handler. As data is read, the handler will be called with the data.
   *
   * @return a reference to this, so the API can be used fluently
   */
  @Fluent
  ReadStream<T> handler(@Nullable Handler<T> handler);

So, it doesn't tell us what to do with the Buffer.

[geoand]

My question is whether in this case the buffer will ever be reused.
We control the handling completely, no?

[FroMage]

To give you an example of why this sort of thing should be specified by the javadoc, here's where this handler gets called in the vertx http client:

  void handleChunk(Buffer chunk) {
    Handler<Buffer> handler = chunkHandler;
    if (handler != null) {
      context.dispatch(chunk, handler);
    }
    if (body != null) {
      body.appendBuffer(chunk);
    }
  }

You can see our handler is called first, then we append the chunk (buffer) to the body field, but if it's been released by our handler, it won't work. I don't know if we're using body but it's just not trivial to guess who the hell should release this buffer.

[geoand]

But currently it seems like no one is, and therefore we get a leak

[FroMage]

Well, it looks like it, I agree, but as you can see from a quick glance at the docs it's not specified, and at the impl, it appears to not always allow us to release it. So it's not clear that this fix is right. If it is right, then the docs should be updated and that's a vert.x bug, and I suspect the body.append call is also a bug. If it's not right, then perhaps it's vert.x which should release this buffer.

My point is that this may appear to work, and may work, but we should raise this question to the vert.x team.

@tsegismont or @vietj can you tell us what we should do with the Buffer passed to a Handler<Buffer>? Release it or not?

[FroMage]

Now, I still don't know who's creating those Buffer but I can see that the stream gets originally buffered in InboundBuffer and the first handlers are called from there. And it's queued in a ArrayDeque<E> pending containing (among other things) instances of Buffer. So logic dictates that since we can queue those buffers, they can't be reused for each chunk by whoever is sending us those notifications. So I tend to think your fix sounds right, as long as this body thing is never used.

This does raise the bigger question of what we should do for other Handler<Buffer> we have. I'm sure we have a few.

[tsegismont]

@geoand @FroMage it doesn't look good to me. Where are the underlying ByteBuf allocated? In Vert.x we use pooled direct buffers only when they do not escape to user code.

I don't know where the corresponding HttpClient is created, but if it can be invoked by users directly, it would leak byteBuf as well I suppose.

[geoand]

I don't know where the corresponding HttpClient is created, but if it can be invoked by users directly, it would leak byteBuf as well I suppose.

It can not - it's an implementatio detail of the REST Client

[tsegismont]

@geoand what happens if the developer uses the REST Client for something else than SSE? Is the direct buffer released somewhere?

[geoand]

The handler in question only comes into play for SSE - none of the other codepaths handle buffers directly

[tsegismont]

@geoand ok then

[geoand]

@FroMage ^

[FroMage]

Well, @tsegismont cleared it up, looks safe then.

[FroMage]

But really, the Vert.x docs could say what handlers should do with buffers they receive.

[tsegismont]

But really, the Vert.x docs could say what handlers should do with buffers they receive.

See comment above, the docs don't say anything because Vert.x does not provide users with pooled buffers. So this case never happens.