fix(coderd): disable websocket compression for startup logs on Safari #8070
Conversation
| // * https://github.com/nhooyr/websocket/issues/218 | ||
| // * https://github.com/gobwas/ws/issues/169 | ||
| ua := strings.ToLower(r.Header.Get("User-Agent")) | ||
| safari := strings.Contains(ua, "safari") && !strings.Contains(ua, "chrome") && !strings.Contains(ua, "android") |
There was a problem hiding this comment.
Probably need to fix this check, I didn't really investigate its completeness.
There was a problem hiding this comment.
On second thought, it seems like a futile effort to detect browser via UA (isn't that the point these days). Should we just always disable compression? (It's really unfortunate, but since we don't control the length of log lines, a single log entry could be too big.)
There was a problem hiding this comment.
+1
I don't think that this check will be bullet-proof, so maybe just set the websocket.CompressionDisabled. If we want to keep the User Agent detection logic, I would look for a Go library to do this.
There was a problem hiding this comment.
Indeed. I think it'd be more robust to detect the browser in the.. browser 😄, so perhaps we could do it in the site package via JS and have the client request compression enabled/disabled. We already include the ua-parser-js library there (for audit logs), so it won't add bloat.
As can be seen here: https://faisalman.github.io/ua-parser-js/, it works well.
|
Closing in favor of #8087. |
There is a problem with Safari and using compression with fragmented
messages. Unfortunately nhooyr/websocket does not support controlling
fragmentation or buffer sizes, so we have to resort to either sending
fixed size payloads or disabling compression for Safari. Given the
variable size of log lines and how well they compress, we choose to
disable compression for Safari.
Before this fix, we may see the following error when the API a large
payload of logs (confirmed using Safari 16.5):
See: