Don't increment bytesRead or unrefTimers if socket is paused

[mcelrath]

When TCP buffers get full such that this.push(buffer) fails, the onReceive handler will be called again with the same data. But because chrome-net increments its buffers and calls _unrefTimer() anyway, even if the data was not accepted into the stream's buffer.

The result is a corrupted data stream, if the sender ever fills your TCP buffer fully and has to back off.

The following patch fixes that.

Also added a couple of missing error codes...

[feross]

When TCP buffers get full such that this.push(buffer) fails

As I understand it, this.push() can never fail. The return value it provides is merely advisory.

Can you actually provide a test case where data is being dropped?

[mcelrath]

Aaaaahhh this doesn't actually solve my problem. I just got lucky and didn't see it for a while.

The behavior I'm seeing is that the application code is seeing bytes from the network truncated, at the beginning of a packet, after his buffer gets full. @feross if this gives you any ideas...I'm pulling my hair out...

FYI for me the underlying chrome apps code is https://github.com/MobileChromeApps/cordova-plugin-chrome-apps-sockets-tcp and I've also pulled in this patch, which fixes one bug: MobileChromeApps/cordova-plugin-chrome-apps-sockets-tcp#25

[mcelrath]

e.g. immediately after the call to chrome.sockets.tcp.setPaused() (corresponding to a full receive buffer at 32kb), the next packet will see its first few bytes truncated.

[feross]

Maybe it's a bug in Chrome's implementation of setPaused? What happens if you just comment that out (i.e. removing backpressure)?

[mcelrath]

removing setPaused doesn't change anything. :-(

[mcelrath]

Packets are getting reordered in the presence of TCP Window Full events. Below is a printout of data from wireshark and the datastream my app received from chrome-net:

printit(lengths, ws, ds)
offset        0: ws |fabfb5da696e7600	 ds |fabfb5da696e7600
offset     1472: ws b1580a020000004f|314e67b3f9a2dab2	 ds b1580a020000004f|314e67b3f9a2dab2
offset     2920: ws bc2bdca8d063b8c3|2b4957c8f718dc39	 ds bc2bdca8d063b8c3|1b50bbb5a6613c6a
offset     4368: ws f9d3da5b4c28c34b|1b50bbb5a6613c6a	 ds 873eaffd178df798|2b4957c8f718dc39
offset     5816: ws 873eaffd178df798|8b3a9b2bcca36f02	 ds f9d3da5b4c28c34b|8b3a9b2bcca36f02
offset     7264: ws 292fc5406b7f0e02|0000002868810b07	 ds 292fc5406b7f0e02|0000002868810b07
offset     8712: ws 0000003e5dd0d38c|9a7aa4c730e23dc1	 ds 0000003e5dd0d38c|9a7aa4c730e23dc1
offset    10160: ws d6f78edbd141b8d1|c2dcd2c2f7d2737b	 ds d6f78edbd141b8d1|c2dcd2c2f7d2737b
offset    11608: ws b589af5a3124bebd|04104c7d6c994608	 ds b589af5a3124bebd|04104c7d6c994608
offset    13056: ws e160782c572176cb|806847020000007f	 ds e160782c572176cb|806847020000007f
offset    14504: ws 41af5102000000c8|9972ab5cf38bea38	 ds 41af5102000000c8|9972ab5cf38bea38
offset    15952: ws 66f7356810902a12|723ebc4f1a9852e1	 ds 66f7356810902a12|723ebc4f1a9852e1
offset    17400: ws ce0be47145f6e698|b2bc26365d57f3ea	 ds ce0be47145f6e698|b2bc26365d57f3ea

The pipe symbols are the locations of packet boundaries in the data stream. As you can see, the packets at offset 2920 and 4368 got reordered.

Still not sure if this is chrome-net's problem or the underlying TCP library.

[mcelrath]

Seems the problems was in cordova-plugin-chrome-apps-sockets-tcp calling readyToRead in its data handler.

[feross]

Thanks for investigating.

On Thu, Mar 2, 2017 at 2:29 PM Bob McElrath ***@***.***> wrote: Seems the problems was in cordova-plugin-chrome-apps-sockets-tcp calling readyToRead in its data handler. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#31 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHbpiYjeWQLxLajwE2l9kjRGpAqKkjiks5rh0LQgaJpZM4MQG0_> .

-- Feross 📱Sent from mobile

[mcelrath]

FYI, actual fix for this problem is here: MobileChromeApps/cordova-plugin-chrome-apps-sockets-tcp#27

Closing...