Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Async server example and constructor with rx_remainder for async framer #19

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

Conversation

dbdbc
Copy link

@dbdbc dbdbc commented Dec 15, 2023

Added an async server example.

The newly added constructor was required because read_header already reads from the stream before the websocket upgrade even happens.

Also, I fixed some stuff clippy was not satisfied with.

When looking at the client_async example, I noticed that the same buffer is used for rx and tx. While this might be fine in this simple case, I think in more general scenarios it could violate the contract described in framer_async::Framer::read

NOTE: any unused bytes read from the stream but not decoded are stored at the end
of the buffer to be used next time this read function is called. This also applies to
any unused bytes read when the connect handshake was made. Therefore it is important that
the caller does not clear this buffer between calls or use it for anthing other than reads.

This should in my opinion not be in an example in this way, but to have similar implementations for client_async and server_async, I also reused the same buffer for reading and writing.

Are there ways to enforce the mentioned contract? E. g. by not only storing indices into the buffer (frame_cursor, rx_remainder_len), but also a &mut [u8] in the Framer struct? Or would this conflict with the design you have in mind?

Anyway, that is not really part of this PR.

Thank you for creating this repo, and have a nice day!

}
Err(e) => {
return Err(FramerError::WebSocket(e));
match stream.next().await {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happened to the loop? I believe that we need it if we only receive tiny chunks of data from the stream, perhaps not enough to read everything we need to complete the connect operation.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The loop is never used, because we either return or panic inside it. However I now see that you probably wanted to handle the HttpHeaderIncomplete in the future instead of panicking, so if you want me to I can undo that change.

@ninjasource
Copy link
Owner

ninjasource commented Dec 20, 2023

Hi, thanks for taking the time to bring this issue to my attention and propose a solution. I'm going to play around with your changes to see if I like them. Very good point about reuse of that buffer for reads and writes, I'll have to give it some thought.

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

Successfully merging this pull request may close these issues.

None yet

2 participants