eidheim / Simple-Web-Server

A very simple, fast, multithreaded, platform independent HTTP and HTTPS server and client library implemented using C++11 and Boost.Asio. Created to be an easy way to make REST resources available from C++ applications.
MIT License
2.61k stars 751 forks source link

Support multiple calls to Response::send #229

Open z0u opened 6 years ago

z0u commented 6 years ago

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.

Type1J commented 6 years ago

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.

z0u commented 6 years ago

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?

eidheim commented 6 years ago

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 commented 6 years ago

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).

Type1J commented 6 years ago

@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.

eidheim commented 6 years ago

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.

z0u commented 6 years ago

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.

eidheim commented 6 years ago

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

eidheim commented 6 years ago

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).