Update uploader() to slow down initial message doubling

[robertodauria]

This PR works around the Safari bug where bufferedAmount isn't updated immediately after a sock.send() and thus causes:

• the send loop to spin extremely fast at the beginning (when messages are small)
• too many sock.send() in a very short time
• the test itself crashing with a Failed to send WebSocket frame error because it tried sending more data than the WS buffer can contain.

By removing the loop in the uploader function no more than one send() can happen in a single call to uploader(), and this slightly slows down the beginning of the upload. It also changes the logic from:

• On each call to uploader(), completely fill the buffer

to:

• On each call to uploader(), queue one more message if the buffer is not full already

Additionally, by looking at WebKit's source code I noticed that the maximum size for the WebSocket buffer is 100MB. By reducing maxMessageSize from 16M to 8M we keep the buffer size below that value and also have some overhead for when bufferedAmount is updated late. The (new) maximum buffer size is 8 * 8MB - 1 byte ~= 64M.

The combination of these two changes seems to be enough for Safari not to crash. This has been tested under different simulated network speed and latency conditions.

Please note that this is just a workaround. I believe a "real" solution would involve changing WebKit so that bufferedAmount is always reported correctly, likely by synchronously incrementing it after a sock.send() and before returning to the JS engine. This also seems to be how Chrome's and Firefox's WebSocket implementations work since this issue could not be replicated there -- bufferedAmount immediately increases at the beginning of the upload, right after the first sock.send().

Closes #44.

---

This change is 

[stephen-soltesz]

Reviewed 3 of 4 files at r1.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @robertodauria)

---

src/ndt7-upload-worker.js, line 61 at r1 (raw file):

   * time and maintain the invariant that the message size is always either 8k
   * or less than 1/8 of the total number of bytes we have enqueued. In an

Is this still accurate?

---

src/ndt7-upload-worker.js, line 88 at r1 (raw file):

    // Message size is doubled every 16 messages, up to maxMessageSize.
    if (data.length < maxMessageSize &&
      data.length < (total - sock.bufferedAmount) / 16) {

I find this logic difficult to follow.

total includes all earlier data lengths, right?

The original logic (also a little hard to follow) made a strict threshold for "nextSizeIncrement" independent of total.

Model of current logic in Python (assuming bufferedAmount == 0).

b = 8192                                                                        
maxMessageSize = 8388608 # /* = (1<<23) = 8MB */                                
total = 0                                                                       
i = 0                                                                           
                                                                                
while i < 100:                                                                  
    i+=1                                                                        
    if (b < maxMessageSize and b < (total) / 16):                               
        b = b*2                                                                 
    print(i, b, total, total/16)                                                
    total += b    

[robertodauria]

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @stephen-soltesz)

---

src/ndt7-upload-worker.js, line 61 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

   * time and maintain the invariant that the message size is always either 8k
   * or less than 1/8 of the total number of bytes we have enqueued. In an

Is this still accurate?

I think it's still accurate unless the JS event loop can't keep up with the network -- but that was true for the previous code, too.

---

src/ndt7-upload-worker.js, line 88 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

I find this logic difficult to follow.

total includes all earlier data lengths, right?

The original logic (also a little hard to follow) made a strict threshold for "nextSizeIncrement" independent of total.

Model of current logic in Python (assuming bufferedAmount == 0).

b = 8192                                                                        
maxMessageSize = 8388608 # /* = (1<<23) = 8MB */                                
total = 0                                                                       
i = 0                                                                           
                                                                                
while i < 100:                                                                  
    i+=1                                                                        
    if (b < maxMessageSize and b < (total) / 16):                               
        b = b*2                                                                 
    print(i, b, total, total/16)                                                
    total += b    

I've changed it to pre-calculate nextSizeIncrement. As discussed on Slack/VC, I also think it's a good idea to consider bufferedAmount here (the previous code didn't). If the network isn't fast enough to transfer the buffer's content, we don't want to keep doubling the message size. (total - sock.bufferedAmount) tells us how much data has been actually sent through the network (vs how much data has been queued)

[stephen-soltesz]

Reviewable status: complete! 1 of 1 approvals obtained

---

src/ndt7-upload-worker.js, line 88 at r1 (raw file):

Previously, robertodauria (Roberto D'Auria) wrote…

I've changed it to pre-calculate nextSizeIncrement. As discussed on Slack/VC, I also think it's a good idea to consider bufferedAmount here (the previous code didn't). If the network isn't fast enough to transfer the buffer's content, we don't want to keep doubling the message size. (total - sock.bufferedAmount) tells us how much data has been actually sent through the network (vs how much data has been queued)

Thank you!

[RA80533]

Is the Slack open to contributors outside of the organization?

[laiyi-ohlsen]

Hi @RA80533, definitely. Thank you for submitting the PRs you have. We're working on refining our process for reviewing external contributions outside of the core contributors team, but in the mean time, if you're interested in contributing more, could you reach out to me at laiyi@measurementlab.net? Thanks!