eventsource
eventsource copied to clipboard
donovanhide
Check if a stream is closed just before writing to the stream channels
I ran into this issue a few times this morning. I was getting panics on writes to closed channels because the initial closed check passed in the loop but once it came time to write the event to the channel the stream had been closed. This takes a lock before writing to check that the stream is still open and then holds on to it during the write so it can't be closed between the closed check and the actual write.
Thanks! I guess to be thorough you'd need to protect this send to a channel as well: https://github.com/aphistic/eventsource/blob/06183ec6ef439852031159a0ee9ba8f956138dfc/stream.go#L209
It does seem that mutex is being referred to a lot :-) A better (more Go-ish) design is probably to have a single goroutine writing to the Event and Error channels, and Close() sends a signal over another channel to stop sending. I don't have time to look into it, but if you fancy exploring, go for it :-)
Ah, good point about the retry error writing. I missed that one. I didn't want to fundamentally change how the stream client worked but if you don't mind I could probably take a look at some point.
https://github.com/donovanhide/eventsource/pull/36 offers an alternate approach.
I ran into an issue today with response bodies not being closed when a stream is closed so I just went ahead and rewrote the Stream internals to use a goroutine and channels.
Let me know what you think and if you want me to make any changes!
Two PRS for the price of one :-)
https://github.com/donovanhide/eventsource/pull/36
To be honest, I don't actually use this package at all anymore. My only concern is that changes affect users in subtle ways and the most important thing is to test for all the possible things people have encountered in the various forks of this package. I don't have the time to do so myself, and I know that writing the tests is often harder than the fun code itself. Maybe you and @ashanbrown can collaborate on a Stream implementation and the relevant tests and when you are both happy, I'll push the merge button :-)
As I was going through with moving the code to the goroutine I did think about wanting to change the Decoder into something that returns channel of events instead of blocking (so it's easy to time out while waiting for a decode), so there are some bigger changes I think might be good. If you're concerned with affecting existing users too much (and I completely understand why) it might be best if I just started from scratch and didn't have to worry about breaking interfaces.
Yep, this library was written well before context.Context had even been imagined :-) You have my blessing to rip out any parts of this library that are useful and I'll even stick a link on the README to your repo if you do a good job :-)