dotnet-eventsource icon indicating copy to clipboard operation
dotnet-eventsource copied to clipboard

LaunchDarkly.EventSource 6.0 rewrite, part 1

Open eli-darkly opened this issue 2 years ago • 0 comments

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.

eli-darkly avatar Jan 18 '23 19:01 eli-darkly