464917 - Quit message gets mis-ordered with others in the same packet

Status: RESOLVED FIXED

(Other Applications :: ChatZilla, defect)

Description

[James Ross]

This is mostly apparent when other people connect to the IRC server directly from the server itself, and do a reconnect (so we get QUIT + JOIN is very fast succession). What happens is the QUIT appears *after* the JOIN, which is... well, wrong.

I believe this is because when we get a QUIT, we queue up an event for every channel they were on, but as the QUIT and JOIN arrived together, the new QUITs are queued after the JOIN - so they appear backwards!

Comment 1

[James Ross]

Attachment: Route multiplied events instead of queue

I've not tested this yet, but it's a simple change to review; I'll be testing it over the next few days.

Comment 2

[:Gijs (he/him)]

I seem to remember that some other code simply forcibly adds the events to the front of the queue, rather than the back. The difference is that in that case, the current event gets run to completion, whereas in your patch it doesn't.


With your patch, there is the potential that state differences between the different events get mixed up (eg. tab dragging - which is probably not relevant here, but I can't think of a better example at the moment), and we will have the event manager routing two events at once. I don't really particularly like that idea, without looking at the actual code of the event manager. It's quite possible that it's completely fine with it, but I would prefer the solution indicated in the first paragraph of this comment, if it is indeed a solution (I haven't checked the code). It would seem less "hacky", too...

Comment 3

[James Ross]

Well, the core code already does this: CIRCServer.prototype.onStreamDataAvailable calls routeEvent().

There's also no interface to the eventPump that puts items at the front, so I really hope no code is doing that currently. In fact, I think things break horribly if you do do it because the event pump uses a queue pointer.

Comment 4

[:Gijs (he/him)]

Comment on attachment 348186 [details] [diff] [review]
Route multiplied events instead of queue

Fair enough! r=me

Comment 5

[James Ross]

Tested and checked in -> FIXED.