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

RESOLVED FIXED

Status

Other Applications
ChatZilla
--
trivial
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: James Ross, Assigned: James Ross)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [cz-0.9.84])

Attachments

(1 attachment)

(Assignee)

Description

9 years ago
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!
(Assignee)

Updated

9 years ago
Summary: Quit message get mis-ordered with others in the same packet → Quit message gets mis-ordered with others in the same packet
(Assignee)

Comment 1

9 years ago
Created attachment 348186 [details] [diff] [review]
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.
Assignee: rginda → silver
Status: NEW → ASSIGNED
Attachment #348186 - Flags: review?(gijskruitbosch+bugs)

Comment 2

9 years ago
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...
(Assignee)

Comment 3

9 years ago
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

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

Fair enough! r=me
Attachment #348186 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 5

9 years ago
Tested and checked in -> FIXED.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Updated

9 years ago
Whiteboard: [cz-0.9.84]
You need to log in before you can comment on or make changes to this bug.