Simple-Web-Server icon indicating copy to clipboard operation
Simple-Web-Server copied to clipboard

Support multiple calls to Response::send

Open z0u opened this issue 6 years ago • 9 comments

Currently, successive calls to Response::send may queue data to be sent twice. For example, the following will send "foo.foo.foo.bar.bar.baz.", when it should only send "foo.bar.baz.":

*response << "foo.";
response->send();
*response << "bar.";
response->send();
*response << "baz.";
response->send();

This pull request fixes the issue by taking a snapshot of the buffer before sending it. It also contains a unit test that fails with the old code but passes with the new. No other unit tests are broken by this code.

z0u avatar May 23 '18 02:05 z0u

Is there a way that this can be done without causing a deep copy? Can there be a move, which would clear out the streambuf? If not, then I'd suggest making the streambuf on the heap with a shared_ptr, copy only the shared_ptr instance, create a new one to replace it, then capture the old one in send()'s handler lambda.

Type1J avatar May 24 '18 19:05 Type1J

It is a shame about the copy, but apparently asio::streambuf doesn't support move semantics. If a std::streambuf were used instead, perhaps std::swap could be used. Any thoughts on that?

z0u avatar May 24 '18 23:05 z0u

As I understand it, you only copy the pointers to the data through:

new_stream << old_stream.rdbuf();

Another issue is that async_write, in your solution, can interleave messages. This can however be solved by using strands and a send_queue as in here: https://github.com/eidheim/Simple-WebSocket-Server/blob/master/server_ws.hpp#L298. I'll have a look at this in a few days. The major problem with this solution though is that one might have to introduce additional dependencies when using boost: boost.coroutine and boost.context if I remember correctly. I'll have a look into this as well. Worst case, we need a build option that enables server-sent events.

eidheim avatar May 25 '18 05:05 eidheim

Actually, new_stream << old_stream.rdbuf(); performs a copy it seems, even though the old_stream's streambuffer is emptied. I guess it is because the streambuffers in the two stream can differ. Anyway, one can place the Response::streambuf in a unique_ptr and move the pointer instead (in this case into a shared_ptr).

eidheim avatar May 25 '18 20:05 eidheim

@eidheim your unique_ptr/shared_ptr solution seems to be the best solution to me. However, unless you're capturing it in multiple lambdas, you might be able to use unique_ptr exclusivly.

Type1J avatar May 27 '18 13:05 Type1J

Just letting you guys know that I have not forgotten this PR. Though I'm contemplating if server sent events should be supported at all, since WebSocket is better suited for such tasks. Additionally, not all browsers supports server sent events.

On the other hand, I have not yet decided, and am still thinking about how to best implement server sent events without adding too much complexity.

eidheim avatar May 30 '18 17:05 eidheim

Thanks @eidheim. In my case, at least in the short term, WebSocket is not an option because the client expects an SSE endpoint.

SSE aside, do you agree that multiple calls to send should be supported — even if we don't claim that it's thread-safe? If so I think stronger guarantees around the behaviour of the buffer are required. For me, the current proposed copy or move of the buffer is fine, because my producer only runs on one thread.

z0u avatar May 31 '18 21:05 z0u

@z0u Your point that multiple send calls should be supported, even without SSE, is good.

eidheim avatar Jun 01 '18 17:06 eidheim

I finally got around to completing the multiple successive sends in https://github.com/eidheim/Simple-Web-Server/commit/987983a396830605473a0ef125e611b854899e25. I put this in the https://github.com/eidheim/Simple-Web-Server/tree/sse branch. This solves the issue of interleaving writes (async_write calls multiple async_write_some).

My main issue is the additional processing and complexity of these changes. Also, older Debian based distros might require additional boost libraries installed. I'll have to check this.

Note that I reverted the LICENSE change. Contributions can be seen in the commit history, and thus I think it is common to leave the LICENSE file unchanged (apart from updating the year).

eidheim avatar Jun 03 '18 11:06 eidheim