Skip to content
This repository has been archived by the owner on Jun 12, 2018. It is now read-only.

Support multiple calls to Response::send #229

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

z0u
Copy link

@z0u z0u commented May 23, 2018

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
Copy link

Type1J commented May 24, 2018

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
Copy link
Author

z0u commented May 24, 2018

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
Copy link
Owner

eidheim commented May 25, 2018

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
Copy link
Owner

eidheim commented May 25, 2018

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
Copy link

Type1J commented May 27, 2018

@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
Copy link
Owner

eidheim commented May 30, 2018

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
Copy link
Author

z0u commented May 31, 2018

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
Copy link
Owner

eidheim commented Jun 1, 2018

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

@eidheim
Copy link
Owner

eidheim commented Jun 3, 2018

I finally got around to completing the multiple successive sends in 987983a. 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).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants