tcp: deprecate TcpStream::try_clone()

[ghost]

Motivation

TcpStream::clone() creates a copy of a TCP stream that may be accidentally tied to a different reactor than the original. See here for a reproducible bug and here for a more elaborate explanation.

Solution

Deprecate TcpStream::try_clone() and nudge users towards TcpStream::split() instead.

Closes #774.

[ghost]

@carllerche In #774 (comment), you said:

the solution would be to deprecate try_clone and to update it to always return an error.

Do we really want to always return an error? I worry that this might break some otherwise working code. A deprecation warning should be enough, I think.

[carllerche]

You will have to merge master once #825 lands to fix the build.

[carllerche]

@stjepang it doesn't actually work as intended today, so any code that uses try_clone is probably already broken.

The issue is that each both stream handles will have separate PollEvented instances but epoll can only register a single token for both, so right now, there isn't any way to notify both stream handles.

In theory, it would be possible to make cloning work, but it would not be simple and add overhead to the non-cloned case. I'd rather better understand the use case for cloning here.

[ghost]

Ok, I've turned try_clone() into an error.

Judging by https://github.com/guydunigo/tokio_test/blob/master/src/main.rs, I think @guydunigo wanted to have one task for writing into TcpStream and one task for reading from it. This is a use case where split() would be appropriate.

[tobz]

LGTM.

[carllerche]

Are there other try_clone fns to deprecate as well?

[ghost]

There is also fs::File::poll_try_clone(). Should we deprecated that one, too?

[carllerche]

If it dups the fd, probably.

[ghost]

@carllerche File::poll_try_clone() dups the fd, but the fd is not tied to a reactor (it uses blocking() instead), so it doesn't suffer from the same problem.

If we ever implement "real" async filesystem IO (e.g. with Linux AIO) and tie it to a reactor, then it will become a problem.

cc @LucioFranco Since you've been reviewing code involving this function, do you perhaps have an opinion?

[LucioFranco]

@stjepang I am indifferent to keeping it but if its going to cause issues later down the line I am fine with removing it. So I see how this might cause issues though with the reactor and how it may cause more confusion down the line. So 👍 on deprecating File::poll_try_clone.

[TheGr8Nik]

I think the message should be TcpStream::try_clone() instead of TcpStream::split()

[ghost]

This was eventually fixed:

tokio/tokio-tcp/src/stream.rs

Line 788 in 847fb59

let msg = "`TcpStream::try_clone()` is deprecated because it doesn't work as intended";