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

Update example code to use tokio-io crate #158

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

stensonowen
Copy link

Compiling the demo no longer complains about a dozen things being
deprecated and should be a little more future-proof. Changes are mostly
identical to an example on tokio.rs.

The server now also handles CRLF line endings, so it can be used with
telnet.

Compiling the demo no longer complains about a dozen things being
deprecated and should be a little more future-proof. Changes are mostly
identical to the example at
https://tokio.rs/docs/getting-started/simple-server/.

The server now also handles CRLF line endings, so it can be used with
telnet.
Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

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

Looks good, I spotted one small problem with writing to BytesMut. I commented below on the line.

src/lib.rs Outdated
//! fn encode(&mut self, item: u64, into: &mut Vec<u8>) -> io::Result<()> {
//! writeln!(into, "{}", item);
//! fn encode(&mut self, item: u64, into: &mut BytesMut) -> io::Result<()> {
//! writeln!(into.writer(), "{}", item)?;
Copy link
Member

Choose a reason for hiding this comment

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

into needs to reserve OR you can use into.extend(...). Otherwise, you could hit a panic trying to write data into the BytesMut when there isn't enough capacity.

Copy link
Author

Choose a reason for hiding this comment

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

Does this cover it?

Otherwise if there's insufficient capacity it apparently could panic
@qhris
Copy link

qhris commented Oct 12, 2017

The comments and texts should probably also be updated here. Currently tokio-core is mentioned twice at lines 58, and 89, prior to the PR, and should be updated or the comments will be misleading.

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.

3 participants