Upload test in Safari overloads buffer

[laiyi-ohlsen]

Symptoms: Client crashes just after the beginning of the upload test

Observations:
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.

[laiyi-ohlsen]

Proposed solution: use a smaller maximum message size and a less aggressive send() loop that doesn't fill the buffer as quickly.

[ahardewig]

Moving comment from another issue to this one:

Hey guys, just wanted to share some findings on this issue. For some background, we've been using ndt for speedtests for a little over a year. We forked the ndt7-js client to add some custom logic. We noticed that ndt7 seemed to fail on safari so, until now, our solution has been to run the ndt5 client for safari and ndt7 for all non-safari. Recently we had some extra dev time so we tried to figure out why ndt5 seemed to work fine on safari and ndt7 seemed to fail. I'll echo some of the above but here's what we were able to discover:

1. Safari websockets don't seem to accurately report sock.bufferedAmount. It appears that it lags behind the true value because we can see it report size 0 for a while after we've been sending data. (We are unsure if its a "slow start" issue where after x amount of time, it accurately reports the value or if its an "eventual consistency" issue where the value is always stale and not necessarily the most up to date.)

2. Websockets terminate if the upload buffer overflows (this is expected behavior in all browsers afaik)

3. NDT7 has adaptive message sizes beginning at 8192 and ending at 16MB. NDT5 appears to send a constant 1MB payload during the upload test.

These 3 behaviors seem to create the perfect storm for Safari. The adaptive message size quickly increases the payload size because sock.bufferedAmount continues to erroneously report size = 0. Because the payload quickly reaches 16MB, there is a good chance that slower clients fail to evict bytes out of the buffer before it overflows, causing the upload test to terminate.

We are still doing additional validation to confirm our fix, but its shown some success so far. We don't want to create lots of extra conditions by turning adaptive message sizes on or off, so instead we've adapted the upload worker to take in a config with an override for maxMessageSize. This essentially makes the change in upload worker limited to a single line
const maxMessageSize = maxMessageSizeOverride ?? 16777216;.

The client consuming the NDT7-js package should determine whether it is being run on safari. If it is, it should pass in a maxMessageSizeOverride that its comfortable with (we are experimenting with 1MB), otherwise it can pass in no config and rely on 16MB maxMessageSize.

We are still trying to perform some additional tests on slower connections to determine if the 1MB max size is sufficient to prevent early terminations but we had lots of success with NDT5 (which sends constant 1MB payloads) and hope this will allow the ndt7 client to run on all browsers.

Additionally, because sock.bufferedAmount is used in the calculation of numBytes, it might be more accurate or safer to use the response from the server. It looks like the TCPInfo object has BytesReceived and ElapsedTime to perform the upload speed calculation. Any thoughts on this approach? Is there anything that I'm missing or overlooking that could negatively impact accuracy or results quality? We'd be happy to take a look at contributing this upstream if this approach is acceptable