SSE with rest-client-reactive may lose message content when it incorrectly breaks the message delimiter (\n\n)

[felixng313]

Describe the bug

The problem arises when the REST client splits the returned SEE message from the remote server into chunks. If it incorrectly breaks the message delimiter (\n\n) into two chunks, it can result in the loss of the message.

Here is an example message I tested:
data: xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\n\n

It will finally be split into two chunks as follows:
chunk 1: xxxx....xxx\n
chunk 2: \n

And here is the rest client example:

@RegisterRestClient(configKey = "test")
interface TestSseIssueService {

    @POST
    @Produces(MediaType.SERVER_SENT_EVENTS)
    @SseEventFilter(TestFilter::class)
    @Path("/test")
    fun test(): Multi<String>

    class TestFilter : Predicate<SseEvent<String?>> {
        override fun test(event: SseEvent<String?>): Boolean {
            println("expect there will show the data content, but actually it's empty.")
            println(event.data())
            return true
        }
    }
}

And here is the breakpoint from the Http1xClientConnection.java
Chunk 1:

Chunk 2:

Expected behavior

No response

Actual behavior

No response

How to Reproduce?

No response

Output of uname -a or ver

No response

Output of java -version

No response

Quarkus version or git rev

3.6.1

Build tool (ie. output of mvnw --version or gradlew --version)

gradle 8.3

Additional information

No response

[quarkus-bot]

/cc @cescoffier (rest-client), @geoand (rest-client)

[geoand]

Is there any chance you can add a sample application that reproduces the problem? It will be very helpful in order debugging and creating a test.

Thanks

[felixng313]

@geoand Here is an example with two test cases: one that is expected and another that is unexpected.

https://github.com/felixng313/quarkus-sse-issue

[geoand]

Thanks

[felixng313]

I created one more test case that randomly fails with streaming output, where each message contains only one character.
Are there any updates on the progress of the fix? It's impacting the production environment.

[geoand]

I have not had time to look into yet. Hopefully early next week

[felixng313]

Update:
I have tested versions 3.6.3, 3.5.3, 3.2.9, and even the previous generation 2.13.9, and all of them have this issue. I have been unable to find a workaround for it.

[cescoffier]

SSEParser:

// FIXME: if our buffer cuts here that's a bad spot

Yes.... exactly that.

[geoand]

@FroMage do you have any bandwidth to fix the parser?

[FroMage]

This looks like we're screwed if HTTP chunks between \r and \n, due to the lax syntax as defined in https://html.spec.whatwg.org/multipage/server-sent-events.html#parsing-an-event-stream:

stream        = [ bom ] *event
event         = *( comment / field ) end-of-line
comment       = colon *any-char end-of-line
field         = 1*name-char [ colon [ space ] *any-char ] end-of-line
end-of-line   = ( cr lf / cr / lf )

So, actually there are lots of ways where chunking would work, such as if it happens within a 2-byte UTF8, because we will detect that something is missing. But if chunking happens in the middle of a field name, or in the middle of a value, it looks like the SSEParser will just discard the event:

data: some[CHUNK]thing\r\n
da[CHUNK]ta\r\n

This will result in:

• the some value being discarded due to not completing a line,
• the thing part being treated as a field name, and discarded (because we discard non-data fields),
• the da field being discarded due to not completing a line, and
• the ta part being treated as a field name, and discarded (because we discard non-data fields).

So, we can't HTTP chunk in the middle of a line, though in this case our parser could be fixed to throw a NeedsMoreDataException and wait for the next chunk to restart.

Now, the case I don't see how we can support is if we chunk in between \r and \n because both are valid line terminators, and so suppose we have the following message:

data: foo\r[CHUNK]\n
data: bar\r\n

Which should append both values and fire an event with value foobar, we will actually treat it as if it were:

data: foo\r
\n
data: bar\r\n

Which means (from the SSE spec), two separate events, of values foo and bar.

Not sure at all how we can detect that, frankly.

But who generates this SSE event? Because we never generate \r\n ourselves, we use \n as separator, which should work.

BTW, \n\n is not a valid end-of-line.

[felixng313]

I'm not talking about \r\n. \r\n is equivalent to a single \n. And \n\n is the standard message delimiter to separate each data. The rest client will not handle it properly when \n\n is split into two chunks.

Please try using my sample reproducer to check the issue.
https://github.com/felixng313/quarkus-sse-issue

[FroMage]

OK, right. So in this case what happens is a variation of the first problematic case that we should fix, where we drop events:

data: foo\n[CHUNK]
\n

What happens is:

• We parse an event with data foo, but we don't fire it and we don't notice that we're not done, so we drop it
• We parse an empty event

Should be the same fix as the first case: if we have stuff to parse, but no firing of event, we should treat it as if we don't have enough data and not drop the input.

[FroMage]

Interesting that the test for this ended up in ./extensions/resteasy-reactive/quarkus-resteasy-reactive-jsonb/deployment/src/test/java/io/quarkus/resteasy/reactive/jsonb/deployment/test/sse/SseParserTest.java of all places 🤷

[felixng313]

Interesting that the test for this ended up in ./extensions/resteasy-reactive/quarkus-resteasy-reactive-jsonb/deployment/src/test/java/io/quarkus/resteasy/reactive/jsonb/deployment/test/sse/SseParserTest.java of all places 🤷

But none of the test cases in SseParserTest.java cover this issue.

I think there should be a new test for this issue, like the following example.

// one event in two buffers with breaking \n\n
testParser(Arrays.asList("data:foo\n", "\n"),
        Collections.singletonList(new InboundSseEventImpl(null, null)
                .setData("foo")));

[FroMage]

But none of the test cases in SseParserTest.java cover this issue.

I know.

So, we can't HTTP chunk in the middle of a line, though in this case our parser could be fixed to throw a NeedsMoreDataException and wait for the next chunk to restart.

Good news is that this is not true, we already handle chunks in all these cases by requiring bytes until an end-of-line

[FroMage]

Fix should be at #37866