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

LaunchDarkly.EventSource 6.0 rewrite, part 1 #93

Open
wants to merge 26 commits into
base: 6.0
Choose a base branch
from

Conversation

eli-darkly
Copy link
Contributor

@eli-darkly eli-darkly commented Jan 18, 2023

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:

  • Give the caller more control over when and where things are executed, by reversing the overall design from a push model (SSE client runs a worker task which calls event handlers) to a pull model (caller explicitly requests events one at a time, but can run a separate worker task for this if they want to).
  • Properly support asynchronous event handling. The previous version used async I/O, but when it called event handlers, they were regular synchronous void functions.
  • Make it more feasible to implement a "read data for a large event incrementally" mode, which what okhttp-eventsource calls streamEventData. 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 of okhttp-eventsource) the feature doesn't play well with the choice of a push model.
  • Improve separation of concerns in a way that makes the implementation cleaner, more flexible, and easier to test. The retry delay logic now uses a RetryDelayStrategy abstraction, where the default implementation is our usual backoff/jitter; the logic for obtaining an input stream now uses a ConnectStrategy abstraction, where the default implementation is HTTP. There's also an ErrorStrategy abstraction which is fairly similar to the previous ConnectionErrorHandler, but its behavior is now a bit better defined.
  • Fix areas of noncompliance with the SSE spec— primarily that we weren't correctly handling CR-only line endings, which was a side effect of relying on .NET's StreamReader class. This means we can now pass our SSE contract tests, so this PR also adds those tests.

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 return Task (which doesn't mean they have to really be async— synchronous logic can be kept by adding return 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 a MessageEvent 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.

/// This class is not thread-safe.
/// </para>
/// </remarks>
internal class BufferedLineParser
Copy link
Contributor Author

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
Copy link
Contributor Author

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);
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

@eli-darkly eli-darkly marked this pull request as ready for review January 18, 2023 19:59
/// </remarks>
/// <returns>an SSE message</returns>
/// <seealso cref="ReadAnyEventAsync"/>
Task<MessageEvent> ReadMessageAsync();
Copy link
Contributor Author

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.

@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