AuthenticatedSseEventStream.processProgressEvent sometimes emits eventName = "message"
Opened this issue · 8 comments
When processProgressEvent is called with the complete event string (both lines) the event is parsed correctly and it emits an event with the correct event name and data. However, when it is called twice, once with the first line and then again with the second line, the event's name is not persisted between the two consecutive calls.
The variable name in line 198 of AuthenticatedSseEventStream.java is set to the correct event name in the first call to the function, however at the second call it is set to the default: "message" and is emitted that way to the subscriber.
If the requestProgressed function receives a payload with response string of for example:
event: statusEvent\ndata: {"data":"XXX","ttl":"60","published_at":"2016-02-22T22:22:23.687Z","coreid":"blah"}\n
processProgressEvent processes this correctly and the event name is correct as statusEvent
If however requestProgressed receives
event: statusEvent\n on its own followed sometime later by
data: {"data":"XXX","ttl":"60","published_at":"2016-02-22T22:22:23.687Z","coreid":"blah"}\n
because there is no data field in the first response string, dataBuffer remains empty and no event is sent.
Then when it receives the data response string, an event is generated but the eventName has been lost and is replaced with "message" as this is a new call of processProgressEvent
I'm guessing that an event doesn't have to have a name, so it is valid to publish an event with just a data field, and it gets the eventName "message".
The simplest solution here seems to be to include an eventName field in the data field.
e.g.
data:{"eventName":"statusEvent","data":"XXX","ttl":"60","published_at":"2016-02-22T22:22:23.687Z","coreid":"blah"}\n
Yes but this requires changes in the Particle cloud. Wouldn't it be more feasible to handle this in code by persisting the event name between different calls to processProgressEvent?
"Yes but this requires changes in the Particle cloud"
Funny you should say this because I noticed last night that the event reported in Dashboard contains an eventName field in the data as I have it above but as the last field rather than the first.
I was trying to figure out where the eventName is dropped from the data.
Regarding persisting the event name, yes this is possible but dangerous I think. Would it not be possible for an eventName sent alone to be associated with an unrelated data field sent later without an eventName? Maybe not a real scenario but possible all the same.
By the way I noticed that processProgressEvent is invoked regularly with "\n" data. Any idea what that is?
I think the dashboard does this as a sort of convenience after the event is received, because even when using the REST api the event name is always sent on a separate line from the event's JSON.
Yeah it feels like a workaround to me too, but the simplest one. I think this requires more analysis of the SSE code. Or perhaps some help from the maintainers :)?
I've noticed the repeated "\n"s too. It's something the server does by itself, sending empty lines periodically as long as the connection is opened. Noticed it with the REST api too.
According to the spec, the event should be dispatched only after reading a blank line.
So processProgressEvent should store the event and wait for additional stream data before publishing it. I think this is a bug in SSeEventStream and AuthenticatedSseEventStream.
I'm having the same issue. Did anyone solve it?
@jensck what should we do with this issue?
cc @CityVibes
We should replace the less-than-ideal SSE implementation we're using now and replace it with something based on this: https://github.com/heremaps/okhttp/tree/master/okhttp-sse
The SSE stack we're using now has a number of other issues, and now we have a correctness problem to add to the list. What I like to above could be fairly easily adapted and seems to be complete and correct, while using far less code to do it.