-
Notifications
You must be signed in to change notification settings - Fork 16
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
LaunchDarkly.EventSource 6.0 rewrite, part 1 #93
base: 6.0
Are you sure you want to change the base?
Conversation
/// This class is not thread-safe. | ||
/// </para> | ||
/// </remarks> | ||
internal class BufferedLineParser |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic here is a pretty straight port from the equivalent class in okhttp-eventsource
. The differences are basically 1. it uses async I/O and 2. it abstracts the stream with a read function rather than referencing the Stream class (see code comment immediately below).
/// <summary> | ||
/// An internal class containing helper methods to parse Server Sent Event data. | ||
/// </summary> | ||
internal sealed class EventParser |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again this is a pretty direct port of the Java code, but it is smaller because it doesn't yet include any of the "provide a stream for the caller to read event data incrementally" logic.
/// </summary> | ||
internal static string StreamClosedByServer { | ||
get { | ||
return ResourceManager.GetString("StreamClosedByServer", resourceCulture); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how important it is to provide these internationalizable strings— given that 1. we're still only providing English-language text, and 2. it's not something we're doing in any of our other libraries— but since this project has historically had them, I've maintained them.
/// state when appropriate by simply reusing the original instance. | ||
/// </para> | ||
/// </remarks> | ||
public abstract class RetryDelayStrategy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Equivalent to the Java version.
/// Instances of this class should be immutable. | ||
/// </para> | ||
/// <seealso cref="ConfigurationBuilder.ErrorStrategy(ErrorStrategy)"/> | ||
public abstract class ErrorStrategy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Equivalent to the Java version.
/// that they produce is stateful and belongs to a single EventSource. | ||
/// </para> | ||
/// </remarks> | ||
public abstract class ConnectStrategy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Equivalent to the Java version.
/// </remarks> | ||
/// <returns>an SSE message</returns> | ||
/// <seealso cref="ReadAnyEventAsync"/> | ||
Task<MessageEvent> ReadMessageAsync(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One notable API difference between this and the Java okhttp-eventsource
is that the latter also has methods that return messages/events as an Iterable
(Java equivalent of an IEnumerable
), so you can just treat them like any collection instead of explicitly asking for each event. That's convenient, but the problem is that async generators are a C# 8.0 feature, and we're still constrained to use C# 7.3 in order to maintain .NET Standard 2.0 compatibility.
…em to other immutable fields
This is a major rethink of this fairly old code, closely based on the v4.0.0 redesign of our Java SSE client
okhttp-eventsource
. It has the following goals:okhttp-eventsource
callsstreamEventData
. This PR does not implement that feature (that's why it is "part 1"), but it's an important first step toward it, because (as I found when trying to implement it in an earlier version ofokhttp-eventsource
) the feature doesn't play well with the choice of a push model.RetryDelayStrategy
abstraction, where the default implementation is our usual backoff/jitter; the logic for obtaining an input stream now uses aConnectStrategy
abstraction, where the default implementation is HTTP. There's also anErrorStrategy
abstraction which is fairly similar to the previousConnectionErrorHandler
, but its behavior is now a bit better defined.Like the Java equivalent, this PR provides a
BackgroundEventSource
wrapper class that is the equivalent of the old push model, making it relatively straightforward to adapt any code that was using the older version; the main difference is that the event handlers must returnTask
(which doesn't mean they have to really be async— synchronous logic can be kept by addingreturn Task.CompletedTask
).p.s. One thing that still exists in this PR, which does not exist in
okhttp-eventsource
and I think should probably go away, is the idea of being able to specify whether the data field in aMessageEvent
is initially stored as a string or a byte array. The rationale originally was that most applications don't use very large events, and strings are more convenient for them, but those that do (like our SDKs) would prefer to parse the data directly from UTF-8 for efficiency. I think that the latter will be obviated by the addition of an incremental-data-reading mode; applications that expect the data to be very big would use that mode, and everyone else would just treat the data as a string.