Closed Bug 955676 Opened 8 years ago Closed 8 years ago

Don't handle incoming data synchronously

Categories

(Chat Core :: General, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

References

Details

Attachments

(1 file, 12 obsolete files)

*** Original post on bio 2228 at 2013-10-19 17:54:00 UTC ***

*** Due to BzAPI limitations, the initial description is in comment 1 ***
Attached patch WIP (obsolete) — Splinter Review
*** Original post on bio 2228 as attmnt 2967 at 2013-10-19 17:54:00 UTC ***

Handling everything available all at once can cause jank (e.g. infamously during LIST).

This patch adds (yet another) queue, until we have something better. I'm not sure it's the best way to do this or that I haven't overlooked something, but it seems to work.
Attachment #8354748 - Flags: feedback?(florian)
Comment on attachment 8354748 [details] [diff] [review]
WIP

*** Original change on bio 2228 attmnt 2967 at 2013-10-19 17:55:20 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354748 - Flags: feedback?(clokep)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 2228 as attmnt 2968 at 2013-10-19 17:59:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354749 - Flags: feedback?(florian)
Comment on attachment 8354748 [details] [diff] [review]
WIP

*** Original change on bio 2228 attmnt 2967 at 2013-10-19 17:59:20 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354748 - Attachment is obsolete: true
Attachment #8354748 - Flags: feedback?(florian)
Attachment #8354748 - Flags: feedback?(clokep)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 2228 as attmnt 2969 at 2013-10-19 18:15:00 UTC ***

Uses while loop instead of for.
Attachment #8354750 - Flags: feedback?(florian)
Comment on attachment 8354749 [details] [diff] [review]
Patch

*** Original change on bio 2228 attmnt 2968 at 2013-10-19 18:15:30 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354749 - Attachment is obsolete: true
Attachment #8354749 - Flags: feedback?(florian)
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Comment on attachment 8354750 [details] [diff] [review]
Patch

*** Original change on bio 2228 attmnt 2969 at 2013-10-19 18:16:23 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354750 - Flags: feedback?(clokep)
*** Original post on bio 2228 at 2013-10-19 18:35:10 UTC ***

One concern is the state of the prpl when it handles messages from flushQueue() in the case where disconnect() is called by the prpl, i.e. when reportDisconnecting has already been called.
*** Original post on bio 2228 at 2013-10-19 21:46:12 UTC ***

Is there an async socket we can use?  I vaguely recall that, but don't know how it works...
*** Original post on bio 2228 at 2013-10-19 21:52:52 UTC ***

(In reply to comment #4)
> Is there an async socket we can use?  I vaguely recall that, but don't know how
> it works...

What do you mean? It's the way _we_ process the received data that is synchronous.
*** Original post on bio 2228 at 2013-10-21 16:22:58 UTC ***

On the plus side, this also helps when joining channels, as the participant list is now filled without blocking the UI.

On the minus side, there is indeed an issue when disconnecting during LIST ("too much recursion") which needs looking into.
Comment on attachment 8354750 [details] [diff] [review]
Patch

*** Original change on bio 2228 attmnt 2969 at 2013-10-21 19:56:55 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354750 - Flags: feedback?(florian)
Attachment #8354750 - Flags: feedback?(clokep)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 2228 as attmnt 2974 at 2013-10-22 17:20:00 UTC ***

Fixes the recursion issue and an IRC edge case.
Attachment #8354755 - Flags: review?(florian)
Comment on attachment 8354750 [details] [diff] [review]
Patch

*** Original change on bio 2228 attmnt 2969 at 2013-10-22 17:20:28 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354750 - Attachment is obsolete: true
Comment on attachment 8354755 [details] [diff] [review]
Patch

*** Original change on bio 2228 attmnt 2974 at 2013-10-22 17:42:02 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354755 - Flags: review?(clokep)
Comment on attachment 8354755 [details] [diff] [review]
Patch

*** Original change on bio 2228 attmnt 2974 at 2013-10-22 18:34:38 UTC ***

>diff --git a/chat/protocols/irc/irc.js b/chat/protocols/irc/irc.js
>@@ -701,24 +701,33 @@ ircSocket.prototype = {
>   onConnection: function() {
>     this._account._connectionRegistration.call(this._account);
>   },
>   disconnect: function() {
>     if (!this._account)
>       return;
>     Socket.disconnect.call(this);
>     this.onStopRequest = function() {};
>+    this.onConnectionClosed = function() {};
This seems to conflict with defining the function below...what's up with this? I don't see how this can work. Is this OK because we always create a new socket?

>diff --git a/chat/modules/socket.jsm b/chat/modules/socket.jsm
>@@ -376,42 +379,75 @@ const Socket = {
>-        // Send each string to the handle data function
>-        data.forEach(this.onDataReceived, this);
>+        // Add the strings to the queue.
>+        this._pendingData = this._pendingData.concat(data);
>+        if (this._handlingQueue)
>+          return;
>+        this._handlingQueue = true;
>+        this.handleQueue();
I'd prefer this call handleQueue, which does the check and sets to true inside of it? I think.

Thanks for fixing all my comments.

>+  _pendingData: [],
>+  _handlingQueue: false,
>+  // Asynchronously send each string to the handle data function.
>+  handleQueue: function() {
>+    let begin = Date.now();
>+    while (this._pendingData.length) {
>+      this.onDataReceived(this._pendingData.shift());
>+      if (Date.now() - begin > 10)
Can we have a comment saying we only run for 10 ms at a time?

>+  _flushingQueue: false,
>+  // Synchronously handle the remaining data.
>+  flushQueue: function() {
>+    if (!this._handlingQueue || this._flushingQueue)
>+      return;
>+    // Ensure we don't recursively call this function. This would
>+    // happen e.g. if the data itself triggers a disconnect()
>+    // call when handled.
>+    this._flushingQueue = true;
>+    this._pendingData.forEach(this.onDataReceived, this);
>+    this._pendingData = [];
delete this._pendingData;
Attachment #8354755 - Flags: review?(clokep) → review-
*** Original post on bio 2228 at 2013-10-22 18:44:53 UTC ***

(In reply to comment #8)
> >   disconnect: function() {
> >     if (!this._account)
> >       return;
> >     Socket.disconnect.call(this);
> >     this.onStopRequest = function() {};
> >+    this.onConnectionClosed = function() {};
> This seems to conflict with defining the function below...what's up with this?
> I don't see how this can work. Is this OK because we always create a new
> socket?

Yes exactly.
http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/irc.js#1343

I added the line because if onConnectionClosed is called after disconnect(), it will run into trouble as this._account is no longer defined. It's consistent with what we do for onStopRequest.

> >diff --git a/chat/modules/socket.jsm b/chat/modules/socket.jsm
> >@@ -376,42 +379,75 @@ const Socket = {
> >-        // Send each string to the handle data function
> >-        data.forEach(this.onDataReceived, this);
> >+        // Add the strings to the queue.
> >+        this._pendingData = this._pendingData.concat(data);
> >+        if (this._handlingQueue)
> >+          return;
> >+        this._handlingQueue = true;
> >+        this.handleQueue();
> I'd prefer this call handleQueue, which does the check and sets to true inside
> of it? I think.

I either don't understand what you mean or I don't understand how this would work?

> >+  // Synchronously handle the remaining data.
> >+  flushQueue: function() {
> >+    if (!this._handlingQueue || this._flushingQueue)
> >+      return;
> >+    // Ensure we don't recursively call this function. This would
> >+    // happen e.g. if the data itself triggers a disconnect()
> >+    // call when handled.
> >+    this._flushingQueue = true;
> >+    this._pendingData.forEach(this.onDataReceived, this);
> >+    this._pendingData = [];
> delete this._pendingData;

I don't delete this._pendingData because the last handleQueue call may happen later.
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 2228 as attmnt 2975 at 2013-10-22 19:11:00 UTC ***

Add comment as requested.
Attachment #8354756 - Flags: review?(clokep)
Comment on attachment 8354755 [details] [diff] [review]
Patch

*** Original change on bio 2228 attmnt 2974 at 2013-10-22 19:11:06 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354755 - Attachment is obsolete: true
Attachment #8354755 - Flags: review?(florian)
Comment on attachment 8354756 [details] [diff] [review]
Patch

*** Original change on bio 2228 attmnt 2975 at 2013-10-22 19:54:17 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354756 - Flags: review?(florian)
*** Original post on bio 2228 at 2013-10-22 20:29:25 UTC ***

(In reply to comment #9)
> (In reply to comment #8)
> > This seems to conflict with defining the function below...what's up with this?
> > I don't see how this can work. Is this OK because we always create a new
> > socket?
> 
> Yes exactly.
> http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/irc.js#1343
> 
> I added the line because if onConnectionClosed is called after disconnect(), it
> will run into trouble as this._account is no longer defined. It's consistent
> with what we do for onStopRequest.
Can we add a comment to this effect? :) (So I won't ask you about this next time!)

> > >diff --git a/chat/modules/socket.jsm b/chat/modules/socket.jsm
> > >@@ -376,42 +379,75 @@ const Socket = {
> > >-        // Send each string to the handle data function
> > >-        data.forEach(this.onDataReceived, this);
> > >+        // Add the strings to the queue.
> > >+        this._pendingData = this._pendingData.concat(data);
> > >+        if (this._handlingQueue)
> > >+          return;
> > >+        this._handlingQueue = true;
> > >+        this.handleQueue();
> > I'd prefer this call handleQueue, which does the check and sets to true inside
> > of it? I think.
> 
> I either don't understand what you mean or I don't understand how this would
> work?
It would be convenient to share this logic with non-delimited text also (so for binary sockets), in which case I'd like as much of the logic in the handleQueue method as possible. (Also this method should be internal, i.e. _handleQueue.) I think you can check if _handlingQueue is true and then immediately return. If it is not true, you then set it to true.

> > >+  // Synchronously handle the remaining data.
> > >+  flushQueue: function() {
...
> > >+    this._pendingData = [];
> > delete this._pendingData;
> 
> I don't delete this._pendingData because the last handleQueue call may happen
> later.
Why does this matter? this._pendingData.length will be zero and just return, or are you concerned about data being added into the array somehow?
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 2228 as attmnt 2977 at 2013-10-23 17:08:00 UTC ***

(In reply to comment #11)
> > > >diff --git a/chat/modules/socket.jsm b/chat/modules/socket.jsm
> > > >@@ -376,42 +379,75 @@ const Socket = {
> > > >-        // Send each string to the handle data function
> > > >-        data.forEach(this.onDataReceived, this);
> > > >+        // Add the strings to the queue.
> > > >+        this._pendingData = this._pendingData.concat(data);
> > > >+        if (this._handlingQueue)
> > > >+          return;
> > > >+        this._handlingQueue = true;
> > > >+        this.handleQueue();
> > > I'd prefer this call handleQueue, which does the check and sets to true inside
> > > of it? I think.
> > 
> > I either don't understand what you mean or I don't understand how this would
> > work?
> It would be convenient to share this logic with non-delimited text also (so for
> binary sockets), in which case I'd like as much of the logic in the handleQueue
> method as possible. (Also this method should be internal, i.e. _handleQueue.) I
> think you can check if _handlingQueue is true and then immediately return. If
> it is not true, you then set it to true.

There is no way for _handleQueue to know whether it is being called from an executeSoon (and should run) or whether it is called because data has just been added (and should only run if the queue was empty before). Of course there would be ways to fix that, but I think you may like the change in this patch ;) Thanks for pointing out that non-delimited strings should be handled as well.

> > > >+  // Synchronously handle the remaining data.
> > > >+  flushQueue: function() {
> ...
> > > >+    this._pendingData = [];
> > > delete this._pendingData;
> > 
> > I don't delete this._pendingData because the last handleQueue call may happen
> > later.
> Why does this matter? this._pendingData.length will be zero and just return, or
> are you concerned about data being added into the array somehow?

Yes, there is no guarantee there isn't still an onDataAvailable call pending.
Attachment #8354758 - Flags: review?(clokep)
Comment on attachment 8354756 [details] [diff] [review]
Patch

*** Original change on bio 2228 attmnt 2975 at 2013-10-23 17:08:21 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354756 - Attachment is obsolete: true
Attachment #8354756 - Flags: review?(florian)
Attachment #8354756 - Flags: review?(clokep)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 2228 as attmnt 2978 at 2013-10-23 18:26:00 UTC ***

(In reply to comment #12)
> > are you concerned about data being added into the array somehow?
> 
> Yes, there is no guarantee there isn't still an onDataAvailable call pending.

And since this actually happens, I guess this means we have to override onDataReceived on the IRC side as well.
Attachment #8354759 - Flags: review?(clokep)
Comment on attachment 8354758 [details] [diff] [review]
Patch

*** Original change on bio 2228 attmnt 2977 at 2013-10-23 18:26:09 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354758 - Attachment is obsolete: true
Attachment #8354758 - Flags: review?(clokep)
Comment on attachment 8354759 [details] [diff] [review]
Patch

*** Original change on bio 2228 attmnt 2978 at 2013-10-24 14:20:32 UTC ***

I'm OK with this change, assuming it doesn't mess up XMPP either. Was this tested?

I do have one nit: can you please add a line break before _pendingData to separate those functions in their own "block". (Feel free to wait until after Florian's review.)

Also, I suspect we only need to call _flushQueue in onStopRequest, that should occur after disconnect gets called. Any particular reason you wanted to call it in both?
Attachment #8354759 - Flags: review?(clokep) → review+
Attachment #8354759 - Flags: review?(florian)
*** Original post on bio 2228 at 2013-10-24 17:17:08 UTC ***

(In reply to comment #14)
> I'm OK with this change, assuming it doesn't mess up XMPP either. Was this
> tested?

I have tested twitter but not JS-XMPP, as I have no suitable accounts to hand. I don't expect problems there though as the signoff procedure is not as convoluted as on IRC ;)
 
> Also, I suspect we only need to call _flushQueue in onStopRequest, that should
> occur after disconnect gets called. Any particular reason you wanted to call it
> in both?

I have no confidence in this, the IRC code calls socket.disconnect in various circumstances and we already explicitly override onStopRequest in that method in case it hasn't happened yet. Depending on what else is going on, the relative timing of the various things that can go on during IRC disconnects is quite variable. Better to keep the _flushQueue call and prevent possible edge cases.
*** Original post on bio 2228 at 2013-10-31 19:45:27 UTC ***

Comment on attachment 8354759 [details] [diff] [review] (bio-attmnt 2978)
Patch

Why is the queue flushed in the disconnect method?

Flushing synchronously in onStopRequest is suboptimal (ideal that should become a message at the end of the queue, so that we don't block the thread if a server drops our connection after sending us plenty of junk), but I guess it's good enough for now.
*** Original post on bio 2228 at 2013-11-01 10:47:46 UTC ***

(In reply to comment #16)
> Comment on attachment 8354759 [details] [diff] [review] (bio-attmnt 2978) [details]
> Patch
> 
> Why is the queue flushed in the disconnect method?

See comment 15.

> Flushing synchronously in onStopRequest is suboptimal (ideal that should become
> a message at the end of the queue, so that we don't block the thread if a
> server drops our connection after sending us plenty of junk), but I guess it's
> good enough for now.

Right, improving that would mean changing the disconnection code in all the protocols. If it turns out to be a problem I can revisit it.
*** Original post on bio 2228 at 2013-11-01 12:14:37 UTC ***

(In reply to comment #17)
> (In reply to comment #16)
> > Comment on attachment 8354759 [details] [diff] [review] (bio-attmnt 2978) [details]
> > Patch
> > 
> > Why is the queue flushed in the disconnect method?
> 
> See comment 15.

Do you have an example of edge case this addresses? Would it be difficult to fix it?

My concern is that this could create other edge cases.
Imagine:
1. As a prpl, I receive junk I can't parse and marks my account is disconnected, and .disconnect() the socket.
2. The socket flushes the queue.
3. As a prpl, I receive more data (junk) while I think I'm already disconnected.

Wouldn't there be edge cases in 3?

It seems to me that calling .disconnect() means "I don't want to exchange any additional data with this socket".
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 2228 as attmnt 2999 at 2013-11-01 17:28:00 UTC ***

(In reply to comment #18)
> It seems to me that calling .disconnect() means "I don't want to exchange any
> additional data with this socket".

After a discussion on IRC, we decided to drop all unhandled data when disconnect() is called on the socket.
Attachment #8354780 - Flags: review?(florian)
Comment on attachment 8354759 [details] [diff] [review]
Patch

*** Original change on bio 2228 attmnt 2978 at 2013-11-01 17:28:19 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354759 - Attachment is obsolete: true
Attachment #8354759 - Flags: review?(florian)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 2228 as attmnt 3000 at 2013-11-01 19:09:00 UTC ***

Removes some (now) dead code.
Attachment #8354781 - Flags: review?(florian)
Comment on attachment 8354780 [details] [diff] [review]
Patch

*** Original change on bio 2228 attmnt 2999 at 2013-11-01 19:09:34 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354780 - Attachment is obsolete: true
Attachment #8354780 - Flags: review?(florian)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 2228 as attmnt 3002 at 2013-11-02 13:54:00 UTC ***

Just in case the stop request status can be zero.
Attachment #8354783 - Flags: review?(florian)
Comment on attachment 8354781 [details] [diff] [review]
Patch

*** Original change on bio 2228 attmnt 3000 at 2013-11-02 13:54:55 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354781 - Attachment is obsolete: true
Attachment #8354781 - Flags: review?(florian)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 2228 as attmnt 3003 at 2013-11-02 15:01:00 UTC ***

Fix indent in activateQueue.
Attachment #8354784 - Flags: review?(florian)
Comment on attachment 8354783 [details] [diff] [review]
Patch

*** Original change on bio 2228 attmnt 3002 at 2013-11-02 15:01:25 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354783 - Attachment is obsolete: true
Attachment #8354783 - Flags: review?(florian)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 2228 as attmnt 3004 at 2013-11-02 15:21:00 UTC ***

Return early if there is no new data.
Attachment #8354785 - Flags: review?(florian)
Comment on attachment 8354784 [details] [diff] [review]
Patch

*** Original change on bio 2228 attmnt 3003 at 2013-11-02 15:21:36 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354784 - Attachment is obsolete: true
Attachment #8354784 - Flags: review?(florian)
Comment on attachment 8354785 [details] [diff] [review]
Patch

*** Original change on bio 2228 attmnt 3004 at 2013-11-02 15:22:53 UTC ***

Seems reasonable. clokep said on IRC he wanted to give this another look.
Attachment #8354785 - Flags: review?(florian) → review+
Attachment #8354785 - Flags: review?(clokep)
Comment on attachment 8354785 [details] [diff] [review]
Patch

*** Original change on bio 2228 attmnt 3004 at 2013-11-02 15:47:53 UTC ***

This makes the socket not re-usable.
Attachment #8354785 - Flags: review?(clokep) → review-
Attached patch PatchSplinter Review
*** Original post on bio 2228 as attmnt 3005 at 2013-11-02 16:19:00 UTC ***

Reusable socket (checks whether we are connected in onDataAvailable instead)
Attachment #8354786 - Flags: review?(clokep)
Comment on attachment 8354785 [details] [diff] [review]
Patch

*** Original change on bio 2228 attmnt 3004 at 2013-11-02 16:19:07 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354785 - Attachment is obsolete: true
Comment on attachment 8354786 [details] [diff] [review]
Patch

*** Original change on bio 2228 attmnt 3005 at 2013-11-02 16:21:07 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354786 - Flags: review?(florian)
Comment on attachment 8354786 [details] [diff] [review]
Patch

*** Original change on bio 2228 attmnt 3005 at 2013-11-02 16:30:25 UTC ***

Looks good to me! Thanks. :)
Attachment #8354786 - Flags: review?(clokep) → review+
Comment on attachment 8354786 [details] [diff] [review]
Patch

*** Original change on bio 2228 attmnt 3005 at 2013-11-02 16:46:38 UTC ***

You can carry forward my previous r+.
Attachment #8354786 - Flags: review?(florian)
Whiteboard: [checkin-needed]
*** Original post on bio 2228 at 2013-11-02 17:03:59 UTC ***

http://hg.instantbird.org/instantbird/rev/9df6ccb0f82d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.5
Depends on: 955700
You need to log in before you can comment on or make changes to this bug.