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

Rename shutdown method on {Tcp,Unix}Stream to shutdown_std and make it private, to avoid clash with AsyncWriteExt::shutdown #3298

Merged
merged 7 commits into from Dec 19, 2020

Conversation

aknuds1
Copy link
Contributor

@aknuds1 aknuds1 commented Dec 19, 2020

Motivation

#3294 describes that the shutdown method should be removed from the {Tcp,Unix}Stream types in favour of the one on AsyncWriteExt. However, plain removing it lead to some compilation failures that were difficult to resolve, so @Darksonn suggested we instead rename the method (to shutdown_std) and make it private. Closes #3294.

Solution

Rename {Tcp,Unix}Stream::shutdown to shutdown_std and make it private.

…yncWriteExt

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
…syncWriteExt

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
@aknuds1 aknuds1 changed the title WIP: Remove shutdown method from TcpStream Rename shutdown method on {Tcp,Unix}Stream to shutdown_std and make it private, to avoid clash with AsyncWriteExt::shutdown Dec 19, 2020
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
@aknuds1 aknuds1 marked this pull request as ready for review December 19, 2020 11:15
@Darksonn Darksonn added A-tokio Area: The main tokio crate M-net Module: tokio/net labels Dec 19, 2020
tokio/src/net/tcp/stream.rs Outdated Show resolved Hide resolved
tokio/src/net/unix/stream.rs Outdated Show resolved Hide resolved
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Copy link
Contributor

@Darksonn Darksonn 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 to me. 👍

@MikailBag
Copy link
Contributor

I think is should be mentioned in docs how one can achieve Read and Both shutdown behaviors.

@Darksonn
Copy link
Contributor

With this change, that isn't possible. However @carllerche suggested on Discord to an .as_std() method that returns a &std::net::TcpStream. It would be possible to access those behaviors through such a method with .as_std().shutdown(how). This would be added in a follow-up PR.

Note that the behavior of shutdown(Read) and shutdown(Both) is platform-dependent and should normally be avoided. The receiver shutting down its read channel doesn't really exist in the tcp protocol in the same way as shutdown(Write) does (as I understand it) and would correspond to an abnormal shutdown of the connection similar to if the connection was lost.

@carllerche carllerche added this to the v1.0 milestone Dec 19, 2020
@carllerche
Copy link
Member

@MikailBag what do you expect happens when shutdown(Read) is called?

@carllerche carllerche merged commit 1b70507 into tokio-rs:master Dec 19, 2020
@MikailBag
Copy link
Contributor

MikailBag commented Dec 19, 2020

I've never used shutdown() so I can't answer this question.

But I think someone else used them. IMHO it'd be good to provide some docs to them, such as "shutdown(Read) is never correct, fix your code" or "use this function", or something else.

This is just an opinion, feel free to ignore :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate M-net Module: tokio/net
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tcp: remove shutdown() fn on TcpStream in favor of AsyncWrite::shutdown()
4 participants