Skip to content
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

Suggestion for simplification and ease of use #63

Open
rupert-madden-abbott opened this issue Nov 3, 2023 · 0 comments
Open

Suggestion for simplification and ease of use #63

rupert-madden-abbott opened this issue Nov 3, 2023 · 0 comments

Comments

@rupert-madden-abbott
Copy link

rupert-madden-abbott commented Nov 3, 2023

Overview
The current algorithm used by this client is to read a certain number of bytes from the response stream until an end of event is detected. At that point the event is parsed and emitted and the remainder of the bytes is added to a buffer. The number of bytes to read defaults to 1024 and is customizable by the user.

At one point in this clients history, a problem was noted when the number of bytes is greater than the size of an event. For example, imagine there is a single event in the stream of 1023 bytes. The read will then block until another byte is emitted from the server which can cause significant delays in the first event being emitted by the client.

In order to remedy this, a change was made to reach for a private variable of the requests library (response.raw._fp), in order to do short reads (read1), instead of reading from the main raw stream. This remedies the above because, now, read1 will attempt to read up to 1024 bytes in a single call and will not block if it cannot read that much, allowing events shorter than this size to be emitted as they are received.

Problems
This all works mostly but there are a number of problems with this approach:

  1. This library now depends on an internal piece of the requests library which makes this library more brittle
  2. response.raw._fp has not been processed by the requests library's chunked transfer encoding handling. This means it cannot be used if this encoding is being used and the client must fall back to using blocking reads
  3. As a user, it is hard to know what chunk size I should specify, especially in the case of blocking reads and events that differ significantly in size.

Proposal
I believe all 3 problems can be fixed by instead reading the stream line by line, instead of with a given number of bytes.

  1. response.raw supports readline so there is no longer any need to rely on response.raw._fp
  2. response.raw has the chunked transfer encoding removed so there is no need for any specific handling when a server emits this.
  3. There is no longer any need to specify a chunk size as the client can just block until an entire line is read.

Since Server Sent Events is a line oriented format, we can be sure it is always safe to block until an entire line is emitted. Consider the case where a server only emits some bytes but without any new lines. In that case, we know that the content cannot contain the end of an event and thus blocking until more bytes are read cannot result in any delayed event deliveries.

Alternatively, for backwards compatibility, a chunk size could continue to be accepted and passed into the readline causing it to return even earlier, if lines exceeded that length but I don't think this would have any practical benefit the option should be discouraged going forwards.

If you are happy with this suggestion, please let me know and I will submit a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant