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

Expose SSE events parser (internal/parser) #39

Open
tmaxmax opened this issue Jul 13, 2024 · 4 comments
Open

Expose SSE events parser (internal/parser) #39

tmaxmax opened this issue Jul 13, 2024 · 4 comments

Comments

@tmaxmax
Copy link
Owner

tmaxmax commented Jul 13, 2024

Hey, I was searching around for only an SSE events parser - encoder/decoder. This was the only library I could find that isn't ancient - thanks for your work!

I'm using an openapi spec which generates ready to use API calls from client side - which is why I am not using the client in go-sse (it would take time to make it play well with openAPI generated code). However, I'm interested in using the code inside internal/parser. I could also help you with exposing it if you had a few ideas in mind.

I saw that the code in https://github.com/donovanhide/eventsource/blob/master/decoder.go, was much simpler and shorter than what you have written up - is there any advantage that I'm missing to your implementation? Please do let me know!

Thanks!

Originally posted by @Pramodh-G in #7 (comment)

@tmaxmax
Copy link
Owner Author

tmaxmax commented Jul 13, 2024

Hi there and thank you for your interest in go-sse! I've created an issue based on your reply to the roadmap so we have a space focused on this discussion only. It's also better for discoverability by others who may have the same question.

First of all, I assume you're looking to decode/encode a wire format representation of an SSE event. go-sse already provides some utilities for that in the exposed API:

Wouldn't these satisfy your use-case or are they too high level? I'm not familiar with what OpenAPI generates – should the already exposed APIs not work for you I could provide better assistance if you wish to elaborate more on how you'd plan to use the internal/parser.

The reason why I'm asking this is because at the moment I wouldn't jump the gun and expose internal/parser as I'd still like to have the flexibility to change the low-level internals without compatibility considerations.

As for the comparison with the code in eventsource, the parser of go-sse closely follows the specification. At a first glance, eventsource doesn't handle line endings and BOM properly – the spec mentions that the line ending can be any of CR/LF/CRLF. go-sse's parser also doesn't drop comments when reading sse.Messages, which may or may not be important to you. Lastly the internal parser is built in a way that it doesn't allocate more than the user allows it to – it can be used (and is used throughout the code) in a zero-alloc manner. Specifically, internal.Parser uses the same buffer for all events parsed and internal.FieldParser does no allocations. In comparison, eventsource allocates for each line.

All in all, go-sse follows the spec, is more flexible and has better performance characteristics.

Finally, I want to thank you for bringing up OpenAPI – it could be an interesting idea to explore a way to integrate go-sse into its generation stack somewhere in the future!

Let me know if this was of help and how I could assist you further!

@Pramodh-G
Copy link

Sorry for the late reply! I've been a bit busy :')

Like you have the WriteTo method, I wanted a ReadFrom method that'd read from a io.Reader or io.ReadCloser. This would ideally require an interface that can read more than a message at a time - so sse.Message.UnmarshalText() might not be the most ideal candidate. I see that internal/parser already has some utilites that do this.

There is SubscribeEvent of the client, but that exposes and underlying http connection - whereas i'd like a raw reader :)

From more of a contributing perspective - I wanted to understand how you want to take this project further. Do you want to improve performance? do you want more UT converage? More integration support (I see that you already have some pub-sub interfaces!)

It would help if we can align so I can contribute to open items!

Thank you so much for the reply :)

@Pramodh-G
Copy link

Disclaimer: While I want to be contributor to the repository, I am definitely not the owner, so I might be wrong/ opinionated here :) . I'm definitely open to having a discussion and reaching a consensus! I feel the structs might need to look like:

  1. Message struct, and methods for that struct that parse text/binary + write to a writer. From your reply above - you already seem to have them :)!

  2. A MessageReader / MessageWriter struct that can read/ write mutiple messages if required from a io.Reader/ io.Writer. We might need this to be separate from the write methods for messages because we would then batch writes instead of writing one message at a time - that might lead to performance boosts.

  3. Both a SSE Client/Server, can use these MessageReader, MessageWriter to write and read SSEs both on client and server. I see that there are both an Event and a Message struct that do contain some shared fields. I do see that an Event is for storing client side state, and a Message is for a server side - but we might get cleaner interfaces when an Event uses a Message internally to parse fields, and some extra fields for the client side intricacies of sse.

Thanks!

@tmaxmax
Copy link
Owner Author

tmaxmax commented Dec 29, 2024

Hi @Pramodh-G! Thank you for your valuable input.

v0.10.0 comes with sse.Read, which gives a way to read sse.Events (so client-side events) from an io.Reader directly. Based on your messages here it seems to satisfy your use case – let me know what you think in #44!

From more of a contributing perspective - I wanted to understand how you want to take this project further.

The roadmap (#7) should be a useful resource for this, as it lists all tasks required to reach a v1 of the library. I try to keep it updated and mark all changes made so it remains relevant. To answer your points directly:

  • Do you want to improve performance?
    Performance of go-sse is generally in a good place now. I do want to create some large-scale benchmarks (for example, a real Joe benchmark) to scientifically confirm that. The updates from v0.9.0 to the message replay logic has also fixed some important and significant bugs and performance issues
  • Do you want more UT converage?
    Coverage is also pretty good right now, at 92%, and I mostly trust the results of the test suite. As I progress developing the library I plan to revise and redo the tests so they cover more edge cases and generally ensure a very high level of safety and certainty. Community feedback tremendously helps here, as through real usage many things are discovered which I couldn't discover by myself
  • More integration support?
    Definitely – someone opened Are there any existing providers out there besides Joe? #19 specifically asking about this and there is also Lets add a NATS provider #13 about it. At the moment I prioritize achieving a minimal, functional and orthogonal public API of the main library first, as this is paramount to having good integrations. For example, it is important to find a way to nicely integrate the use case in How to dispatch messages to individual clients? #36, as that directly affects how third-party pub/sub implementations will look like and behave. Conversely, experience with building integrations would shine light on how such a public API would look like, so in no way am I refusing any contributions on this front.

To conclude this part, out of the three points you raise, external contribution and feedback related to integrations would be the most valuable.

Lastly, to say a few words about your suggestions:

  1. Yes, you are right that Message does what you describe!
  2. MessageReader seems to be sse.Read – unless there's a need to read multiple sse.Messages from an io.Reader, for which frankly I don't see a use case at the moment. A MessageWriter which works with an io.Writer could be useful – right now there is sse.Session, which writes sse.Messages using their WriteTo method to an HTTP connection. I'd like to simplify this part – I'm not really a fan of sse.Session, sse.Server.OnSession and other related parts. Solving How to dispatch messages to individual clients? #36 seems tangential. There's a lot to unpack in this direction.
  3. Your insight into the relationship between Event and Message is very useful and interesting. I'd personally like to go even further and find a way to somehow unify the two – in a way similar to how net/http.Request is used by both the server and the client. Again, it's quite a lot to think through, as I don't want to force this unification and end up with weird decisions and a confusing API. The parsing unification you talk about seems viable today, with the introduction of sse.Read, albeit not by means of using a Message to parse an Event but by using the same internal helper.

This would be it from my side. Thank you for your interest and let me know your thoughts!

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

2 participants