nginx-push-stream-module icon indicating copy to clipboard operation
nginx-push-stream-module copied to clipboard

lp - fix eventid

Open kamil-michalak opened this issue 7 years ago • 3 comments

Long polling - missing eventid parameter

kamil-michalak avatar Dec 04 '17 18:12 kamil-michalak

Hi @kilgor the eventid was added to work with EventSource mode, where while connecting the client "say" what was the last message it received and (ideally) do not disconnect. Of course, it can be used with the other modes as well, but what will happen if not all messages have an event id?! My experience say that the client will receive "duplicate" messages. For example,

  • message 1 has an eventid A
  • message 2 does not have an eventid
  • the client disconnect after receive message 2
  • when the client connect again, with your proposal, it will receive the message 2 again, right?!

Also, can you ensure the current tests for javascript are running and add some more to your changes?

wandenberg avatar Dec 04 '17 20:12 wandenberg

Hi @wandenberg EventId is great if we have more than one publishing server. This is the way to enumarate messages by own publishing system. This paramater work with websocket and eventsource and stream module. I trying to implement own "numbering" messages by this way. I know if i use the same event id i can duplicate messages. I tested that i can remove time and etag paramaters and replace them by eventid. But currently js implementation loose this value.

kamil-michalak avatar Dec 04 '17 20:12 kamil-michalak

Not saying to remove time and etag, just to make them compatible. Something like, when the last received message doesn't have the eventid, erase the value in the pushstream instance. This way, in the next connection only time and tag will be sent to the server and will avoid sending duplicated messages to the client. I am guessing that in your use case all messages have an eventid, but this isn't true to everyone, at least not required. What do you think?

wandenberg avatar Dec 04 '17 21:12 wandenberg