nicolasff / webdis

A Redis HTTP interface with JSON output
https://webd.is
BSD 2-Clause "Simplified" License
2.82k stars 307 forks source link

Ensure websockets write all data; Always keep_alive every websocket #178

Open shawwn opened 3 years ago

shawwn commented 3 years ago

Summary:

Explanation:

I've been using webdis websockets to write large amounts of data to a webgl demo for plotting lots of data points (https://twitter.com/theshawwn/status/1292266967940960257). There were two hurdles:

  1. webdis doesn't handle the return value of write() properly, leading to truncated sends

  2. webdis seems to close websockets after each request.

This PR writes all data, and also marks every websocket as keep_alive = 1, ensuring that it stays open for multiple requests.

There is currently a demo of this here: http://test.tensorfork.com/ though I'm not sure how long that link will remain valid.

shawwn commented 3 years ago

Okay, I'll look into this when I have some time. Your comments were thorough and helpful; thank you.

In particular, terminating the service because a websocket client got disconnected is (slightly) overkill 😊.

It doesn't. In fact, according to the write manpage (https://www.man7.org/linux/man-pages/man2/write.2.html), there don't seem to be any error codes that can cause the err to fire. Most error codes are related to file system errors, for example.

It's a lot more work to handle the error code gracefully. Are you sure there are situations where write can fail in a way that triggers the err?

I agree it's probably better to handle it, but FWIW I've been running this code for a week now with no problems at http://test.tensorfork.com/.

I'm also not sure about this repeated write outside of the event loop. Ideally the call to write(2) should happen when the file descriptor is writable, and the event re-registered if there's still more data to write. This is a more complex change though, and could be done in a second pass.

Yeah, the repeated write is concerning. It opens up the server to a "slow attack", where a client only downloads a few bytes per second. What are some solutions? We could spawn a thread for each connected websocket.