Heartbeating for SSE based subscriptions #2806
Conversation
SvenW
force-pushed
the
fix/sse-heartbeats
branch
from
9071180 to
8406000
Compare
SvenW
force-pushed
the
fix/sse-heartbeats
branch
from
1706163 to
0571d0a
Compare
|
The next release is likely to be 3.0.0 so could you target the |
SvenW
force-pushed
the
fix/sse-heartbeats
branch
from
0571d0a to
5600211
Compare
Of course. Do you already have a planned roadmap for the |
|
I'm just waiting for a new RC of sbt 2 because of #2771 (comment) and it should be good to release |
kyri-petrou
left a comment
kyri-petrou
left a comment
There was a problem hiding this comment.
Thanks! Some minor comments below, although:
- Is it possible to add a test for this or is it too difficult?
- Can we document this new functionality? Both in the scaladoc of the config and the page
| } | ||
|
|
||
| object SseConfig { | ||
| def default: SseConfig = SseConfig(Some(10.seconds)) |
There was a problem hiding this comment.
I'm not sure whether enabling them by default is the correct approach. I think it's better for users to explicitly enable them?
| } | ||
| case _ => ZStream.succeed(resp) | ||
| }).map(v => toSse(v.toResponseValue)) ++ ZStream.succeed(done) | ||
| }).map(v => toSse(v.toResponseValue)).mergeHaltLeft(heartbeater.getOrElse(ZStream.empty)) ++ ZStream.succeed(done) |
There was a problem hiding this comment.
Can we avoid the merging if the heartbeater is None?
Noticed that the TextEventStream test was flaky so didn't look into that yet, but might be worth it?
Added some docs now regarding the configuration at least |
@SvenW I see. I think it's still worth adding a test. I imagine it won't be straightforward to write a test for this, so it'd be good to have one in place. If it turns out to be flaky as well we can ignore it until the underlying bug is resolved |
Agreed. I've added a test now but I realized that sttp doesn't support SSE comments fully either so |
SvenW
force-pushed
the
fix/sse-heartbeats
branch
from
7309689 to
f2453f9
Compare
|
What do you say @kyri-petrou? Do you want me to change anything else? Sorry for the ping btw |
|
@ghostdogpr happy to merge if you're OK with it as well |
|
Let's go, thanks! |
3 participants
This adds heartbeating for SSE subscriptions. With the new zio-http version these heartbeats are now sent as SSE comments (ignored by the client but keeps the connection up) making sure we don't close the connection if no messages are passed within the default timeouts