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

Why message's data is a stream? #13

Closed
lgerardSRI opened this issue Jul 29, 2015 · 20 comments
Closed

Why message's data is a stream? #13

lgerardSRI opened this issue Jul 29, 2015 · 20 comments

Comments

@lgerardSRI
Copy link

From my understanding of your code, each time a message is received, the data is copied to a fresh message object. So the data stored in the message won't change or increase: it could be a simple string instead of an istream. Then, the length field would be superfluous. In fact, the Message class could even inherit from std::string.

The benefits would be a much easier manipulation of the message and most of the time at least one less copy of it.

I could propose a patch if this make sense to you.

@eidheim
Copy link
Owner

eidheim commented Jul 29, 2015

I think I agree with you, will have a closer look Tomorrow.

@lgerardSRI
Copy link
Author

PS: the send function may also be easier to use with a plant string argument.

@eidheim
Copy link
Owner

eidheim commented Jul 30, 2015

Sure, could add a send with string argument. Streams should require less copying though, since strings need to resize when adding to them, while streams add the data at separate locations in memory. Might also be that it is possible to do the masking of data from client within the stream instead of adding the result at different stream or string.

If you see on the client, the Message data is never copied, and this is possible because the message from server is not masked here. Main problem would be that the onmessage would be different on server and client, since the client::onmessage should have its Message as stream to avoid copy to string.

@eidheim
Copy link
Owner

eidheim commented Jul 30, 2015

When I think about the the Message stream could be stringstream I think, that is for both the client and server, and still avoid copy on the client code. Is that satisfactory you think?

@eidheim
Copy link
Owner

eidheim commented Jul 30, 2015

It seems like it is possible to modify the stream buffer directly and thus avoid a copy to a new stream, and then use stringstream as the Message stream on both client and server. I think this is the best solution, just need to subclass a streambuf class to receive the pointers to the chars in a stream directly.

@lgerardSRI
Copy link
Author

I guess you know best, I had a really quick look at your code. I thought things where strange when I saw that (server_ws.hpp:425)

std::ostream message_data_out_stream(&message->data_buffer);
for(size_t c=0;c<length;c++) {
    message_data_out_stream.put(raw_message_data.get()^mask[c%4]);
}

Yes, stringstream would already be a nice improvement.

PS: to prevent string reallocations, one can use std::string::reserve

@eidheim
Copy link
Owner

eidheim commented Jul 30, 2015

I have looked abit more at this, and the solution seems to be a filtered output stream, that is using stringstream with a custom streambuffer (handling the websocket masking of data from client), maybe using Boost.Iostreams. This way one would avoid copy (for instance if one would only look at the first bytes of a Message), and if you'd want a string using stringstream::str() one would get the data only with one copy, as would be the case if the Message stored the data as string.

@alanywlee
Copy link

I looked into code and found it seems not yet support binary data exchange between client and server.
Or just my misunderstanding...

@eidheim
Copy link
Owner

eidheim commented Oct 29, 2015

See https://github.com/eidheim/Simple-WebSocket-Server/blob/master/server_ws.hpp#L149 and https://github.com/eidheim/Simple-WebSocket-Server/blob/master/client_ws.hpp#L93 (client_ws is actually missing that comment, I'll fix that on Saturday). See also #16 that will be fixed then.

The fourth parameter in SocketServer::send and SocketClient::send, fin_rsv_opcode, tells which kind of message it is, 129 for text and 130 for binary. I'll change the comments to doxygen as well so this shows in IDE's supporting this. Binary/text is mostly for UTF-8 parsing I think, so it only affects clients like web browsers that try to make sense of text messages (which should be UTF-8 compatible if I'm not wrong).

@alanywlee
Copy link

Thanks for pointing it out. What I am trying to do to send a photo file from client device to a python based websocket server made by tornado. I am not sure if I need to modify client_ws.hpp to support ifstream. Could you kindly help to show how to do binary data exchange based on this project?

@eidheim
Copy link
Owner

eidheim commented Oct 29, 2015

Based on ws_examples:

echo.onmessage=[&server](shared_ptr<WsServer::Connection> connection, shared_ptr<WsServer::Message> message) {
    auto message_str=message->string();

    cout << "Server: Message received: \"" << message_str << "\" from " << (size_t)connection.get() << endl;

    cout << "Server: Sending message \"" << message_str <<  "\" to " << (size_t)connection.get() << endl;

    auto send_stream=make_shared<WsServer::SendStream>();
    *send_stream << message_str; //instead of message_str, insert raw photo bytes here with for instance send_stream->write
    server.send(connection, send_stream, [](const boost::system::error_code& ec){
        if(ec) {
            cout << "Server: Error sending message. " <<
            //See http://www.boost.org/doc/libs/1_55_0/doc/html/boost_asio/reference.html, Error Codes for error code meanings
                    "Error: " << ec << ", error message: " << ec.message() << endl;
        }
    }, 130);
};

See 130 on one of the last lines, and the comment before server.send.

Edit: I created a server onmessage here, but the client one is very similar.

@alanywlee
Copy link

I followed your instructions to send a photo as the following code:

auto send_stream=make_shared<WsClient::SendStream>();
{
    std::ifstream infile(JPG_FILE, ios::in | ios::binary);
    std::ofstream outfile ("new.jpg",std::ofstream::binary);

    // get size of file
    infile.seekg (0,infile.end);
    long nSize = infile.tellg();
    infile.seekg (0);

    char* buffer = new char[nSize];

    // read content of infile
    infile.read (buffer,nSize);

    // write to outfile
    send_stream->write(buffer, nSize);
    outfile.write (buffer, nSize);

    // release dynamically-allocated memory
    delete[] buffer;
    infile.close();
    outfile.close();
}
client.send(send_stream, nullptr, 130);

But what I received on server is a garbage, but I verified what I saved in new.jpg is exactly the same as my source photo. Did I make anything wrong?

Edit: made it more readable

@eidheim
Copy link
Owner

eidheim commented Oct 29, 2015

Try sending a smaller message to the server and see if you can verify the bytes received there. Try both text and binary messages, in case the server does not support binary messages. If it does not support binary, you can send it as text. In this case it will not matter I think.

@alanywlee
Copy link

I found the binary size received on websocket server is correct, but content is not. I used to use original ws_example to send "Hello" to tornado server and work well. Tornado websocket server was used to work correctly with javascript websocket client in case of photo upload. So I can only doubt if I probably did something wrong.

@eidheim
Copy link
Owner

eidheim commented Oct 29, 2015

Try to send the image as text message (129 instead of 130), and see if there is a difference.

@eidheim
Copy link
Owner

eidheim commented Oct 29, 2015

Also, if you have a link to the python websocket server, I can try this on Saturday and see if I can figure it out.

@alanywlee
Copy link

I did several tests regarding to your suggestions as following:

In case of using 129 (text mode) to send, 0 bytes received and decoded from websocket
server, but 130 (binary mode) can get the whole picture size even content is corrupted.

I also tried to send a text file (100 lines ASCII code with \r\n only) using binary mode,
and found websocket server could correctly receive and decode the text file back.

I also noticed, in case of photo upload, the binary data received on server is different
to different send() using same binary data source. I doubt if the random part might be a problem,
and modified https://github.com/eidheim/Simple-WebSocket-Server/blob/master/client_ws.hpp#L101 to mask[c] = 0xff, and verified even though the photo data I received is still incorrect, but the data I decoded from websocket server would be the same as different send() with same binary source.

I also noticed the lastest version of client_ws.hpp was updated to add some static_cast, but after I update client_ws.hpp to latest version, test result is still the same.

Hopefully, my test results could be a little bit help to figure out what wrong it is.

PS: about the tornado websocket server I referring to, please find package and installation guide in http://www.tornadoweb.org/en/stable/

@eidheim
Copy link
Owner

eidheim commented Nov 1, 2015

I tested the latest client (the changes should not have any effect on this issue, but maybe) on a javax.websocket server using binary messages, and it works on all size variations. However, I had to specify a specific endpoint on the Java server code to handle binary messages, but that was it. Maybe the python server has a similar issue?

Anyway, closing this issue, as its not, at least not now, an issue with Simple-WebSocket-Server.

@eidheim eidheim closed this as completed Nov 1, 2015
@alanywlee
Copy link

I found what I tested before is the lastest released v1.2, and I donwloaded lastest source and tested again, I confirmed the latest source indeed can send binary data to tornado python websocket server without content corruption. Thanks for your support...

@eidheim
Copy link
Owner

eidheim commented Nov 2, 2015

Good to hear, when I think about it, the issue was related to #16. That is, sending multiple sends to actually both server and client could interleave and lead to corrupted messages. Thank you for reporting the issue.

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

No branches or pull requests

3 participants