sseclient 0.0.24 loses events due to Unterminated string... ValueError

[xqt]

sseclient 0.0.24 fails randomly when json.load is processed at the EventSource's data. Randomly means that you can find this failure pretty sure if you process say 50 events from stream. The problem occures with Python 3.7; I've not tested other Python releases. This problem does not occur with sseclient 0.0.22.

I've have implemented a test suite to check this behavior, see https://gerrit.wikimedia.org/r/#/c/pywikibot/core/+/509132/2/tests/eventstreams_tests.py

Additional details you can find on our bug tracker https://phabricator.wikimedia.org/T222885

[xqt]

Digging deeper into this matter, this failure was introduced with 0.0.23 if using this line
chunk = self.resp.raw._fp.fp.read1(self.chunk_size)
ignoring this and using chunk = self.resp.raw._fp.fp.read1(self.chunk_size) my test passes but the sse socket wasn't closed.

[xqt]

I made a pull request PR#29 to check this ugly behaviour

[OverlordQ]

Running into this exact same issue as well with read1 not actually reading full events before passing it off to the handler.

[OverlordQ]

Okay, so I think it comes down to this.

Calling read1 using the raw file-pointer is context agnostic. You say read, it goes gobble gobble and spits back everything it finds.

Problem is, SSE streams require context. If you dump the headers returned from connecting to an SSE Endpoint, you'll probably notice a Transfer-Encoding: chunked header.

What gets pre-pended to every chunk? Size of the chunk.

resp.raw is still a http.client.HTTPResponse object, calling read on that will take into account chunked encoding and strip off the relavent bits and return the actual data.

Using resp.raw._fp.fp.read1 bypasses all of that.

So for example, if you're feeding JSON data through an SSE stream, unless it always matches up perfectly to a chunk, using read1 is going to insert the chunksize and accompanying newlines into the middle of your data, breaking JSON formatting.

There's a reason you shouldn't poke into the guts.

[btubbs]

Thanks for digging into that @OverlordQ.

Looks like we probably need to revert the "short read" change from @mutantmonkey from a few months ago. (See #8 and 8585bac.)

[mutantmonkey]

Regarding Transfer-Encoding: chunked, the Server-Sent Events specification recommends against doing this:

Authors are also cautioned that HTTP chunking can have unexpected negative effects on the reliability of this protocol. Where possible, chunking should be disabled for serving event streams unless the rate of messages is high enough for this not to matter.

If this is the issue, then it sounds like we'll need to handle chunked encoding directly in this library, or detect when it is being used and disable the short read functionality. @OverlordQ Do you happen to have an example SSE endpoint that uses chunked encoding that I would be able to test with?

I understand the concern with poking into the guts, but isn't the purpose of this library to support server-sent events without requiring a low-level understanding of HTTP? There's no other way to properly deal with timely delivery of messages shorter than the specified chunk_size while still using http.client.

[OverlordQ]

I would guess that the Wikimedia test case @xqt gave will exhibit this, but http://stream.pushshift.io/ was where I was running into this behavior.

And yes while it does say should, it doesn't say must, so it is a case that would to be handled

[mutantmonkey]

@OverlordQ I just pushed a branch that falls back to the old method when chunked encoding is enabled. Can you give it a try and confirm that it fixes your issue? https://github.com/mutantmonkey/sseclient/tree/disable_short_reads_when_chunked

Ideally, chunked encoding support should be implemented directly in this library, but that's going to be more involved. Just disabling short reads when using chunked encoding is a much quicker fix since this behavior is broken.

[TheSandDoctor]

@OverlordQ please see the above?