JoakimSoderberg / libws

A multiplatform C websocket client library
MIT License
11 stars 10 forks source link

Fast client writes will fill up memory #4

Open snej opened 7 years ago

snej commented 7 years ago

The client code using libws has no way to tell how fast data is being written to the socket, so it can't tell how fast it can send messages. If it sends messages faster than the socket can write them, the libevent write buffers will start to pile up and increase memory usage. This also increases latency, since the client can end up creating messages that only get written to the socket much later.

(Think of a webcam that wants to send camera frames over the socket as JPEG images. If it starts sending messages at 60FPS, but the network bandwidth isn't enough to support that, then memory will start to fill up with pending frames, and the other side of the connection will see the video feed in slower than real-time. Instead, the webcam should wait until the websocket is ready for more data, and then write the current frame.)

This can be fixed by adding a new callback that tells the client that the socket has room to write. This is equivalent to the write callback provided by libevent; in fact libws's write callback can invoke the new callback.

snej commented 7 years ago

By the way, I'm filing these issues partly as a way to probe whether this project might come back to life — it looks promising (compared to the other libevent-based WebSocket implementations I've looked at) but it's seen no activity for 2 years.

I have fixes for all the issues I've filed, currently in a private fork, but I can submit them as PRs if there's interest.

JoakimSoderberg commented 7 years ago

Thank you for pointing out the issues.

This project was a hobby project of mine to learn libevent and the websockets protocol and API design in a library.

I tried to design it in a way so that it is possible to replace Libevent with any other event library/OS api without having to change the external API. Which in some ways makes things more complicated than they need to be if all you are interested in is to use Libevent. Again this was just a challenge for myself.

However as you have noticed there are a lot of issues still remaining. But I have kind of lost interest in the project myself. A big reason being that I don't really use the websocket protocol in any project. I have never used this library for real, other than my test clients (which explains all the issues you have found). When trying to get full SSL support working I kind of gave up. Because of how bad the OpenSSL init API is, I just did not manage to do it in a nice way which made me lose the "fun" part of it all. But apparently the new verisons have a new API that doesn't make it crash because you init it more than once in an application :)

Here is a fork that has done some substantial changes that removes the "hiding" of Libevent and exposes things: https://github.com/alxvasilev/libws It seems to still be actively being developed. But I have not gotten any Pull Requests, or had any contact with this dev, so it can be considered a standalone fork.

However, if you have fixes for these issues I am glad to accept Pull Requests and merge them. If you are interested in becoming an active maintainer of the project I'm up for that as well. But I don't want to give you any false hopes that I might start maintaining it actively myself, since I have too many other projects going

alxvasilev commented 7 years ago

Hello from the author of the mentioned fork :) First of all, a big thanks to Joakim for his neat library. I have reviewed quite a few websocket client libs, and this was was the best for our needs. We have brought the codebase to a state where it fits our needs and is flexible enought to replace libevent with another library of that type. But at the moment we unfortunately don't have much time to review and accept patches. This may happen in the future, as we actually replace libevent and maybe do another round of fixes/changes to the code. Until then, unfortunately, I am unable to do maintenance of our fork of the library. Joakim, if you are interested, I can do a MR with the changes we have made, just let me know. I didn't think you would accept them as they are, that's why I didn't do it. Cheers Alex

JoakimSoderberg commented 7 years ago

@alxvasilev hello, thanks for the reply. I have looked at some of your changes and I like many of your fixes. It's fun to see my hobby project actually has been useful for someone.

But yea there are some parts of the fixes I would not merge right away. So you don't need to do a PR. But maybe I get some motivation to port parts of your changes back upstream some day :) Hopefully in the process making things acceptable for you to use "as is". Specifically I saw you removed the autobahn test suite. I guess since it requires additional dependencies? I find it good to have such tests to verify that any changes still adheres to the Websocket standard.