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

add streaming event data mode (6.0 rewrite, part 2) #94

Open
wants to merge 4 commits into
base: eb/sc-183031/rewrite-with-pull-model
Choose a base branch
from

Conversation

eli-darkly
Copy link
Contributor

This adds the incremental loading feature that was one of the main motivations for the whole rewrite. As before, the implementation is closely based on the Java version in okhttp-eventsource. Briefly:

  • By default, everything works the same as before except that there is no longer any "store the message data as a reference to an internal byte buffer" behavior— it just stores the data as a string. The intention is that if an application is going to be reading large enough data that the string type would be inefficient, then they will use this new mode instead.
  • If you set StreamEventData(true), then when you receive a MessageEvent it contains a special stream, accessed via the DataStream property. When you read from this stream, it is really reading from the EventParser, consuming the value of every data: field up till it hits a blank line, at which point that stream returns EOF.
  • As soon as you request another event, the DataStream for the previous event becomes invalid, and EventParser skips over any content that hasn't yet been consumed from that one.

One wrinkle with this, which is a necessary departure from the SSE spec, is that EventParser has to return the MessageEvent before it has seen anything past the first data: line. So at that point, it doesn't yet know for sure that there really will be a full event before the end of the stream— and, if any other fields like event: or id: appear after data:, it can't put them into the MessageEvent. This is not an issue for using EventSource in the LaunchDarkly SDKs, because 1. until all of the data is consumed we won't actually be storing any of it, just parsing it; 2. the LD services always send event: before data:; and 3. with the new ExpectFields option, the SDK can specify that it definitely needs to see an event: field and so if for some reason the fields were sent out of order, EventSource will just go back to buffering the full event like before.

/// If so, reading <see cref="MessageEvent.DataUtf8Bytes"/> will return the same
/// byte array, to avoid unnecessary copying.
/// </remarks>
public struct Utf8ByteSpan
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is no longer a public type because MessageEvent no longer has a DataUtf8Bytes property. It's now in Internal and is used only as a convenience container for a tuple of (buffer, offset, length).

_taskFactory.StartNew(taskFn)
.Unwrap()
.GetAwaiter()
.GetResult();
Copy link
Contributor Author

@eli-darkly eli-darkly Jan 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is used only to implement the synchronous Read method in the special stream provided by EventParser. The implementation is borrowed from LaunchDarkly.InternalSdk.

@@ -1,9 +1,11 @@
using System;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is now extremely close to the Java version. The differences are mainly due to the .NET async model, and some other slight differences between .NET's Stream type and Java's InputStream type (for instance, in Java you return -1 for EOF, but in .NET you return 0).

@eli-darkly eli-darkly marked this pull request as ready for review January 19, 2023 20:24
@launchdarkly launchdarkly deleted a comment from shortcut-integration bot Jan 19, 2023
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

Successfully merging this pull request may close these issues.

1 participant