[tcp] Port-reuse, async-io, if-watch #1887

romanb · 2020-12-14T14:06:21Z

This PR is a continuation of #1845. To recap the main changes:

Permit port reuse for local sockets, primarily to facilitate better NAT traversal. In this context address_translation is moved back to the Transport trait, where it already existed at some point in the past, to allow transport-specific translations, as now used for the TCP transport.
Use async-io instead of macro-based code generation to support both tokio and async-std with less boilerplate and for easier code maintenance, thereby removing the need for the respective feature flags, similarly to how it was already done for libp2p-mdns.
In libp2p-tcp, when listening on all interfaces, instead of regularly querying the interfaces with get_if_addrs to be aware of both new and expired addresses, use if-watch to be actively notified of interface addresses as they go up/down.

Compared to #1845 I made the following changes:

TcpConfig::new() is no longer async and it creates neither background task nor thread. If port reuse is enabled on TcpConfig (and only then), the listen socket addresses are registered for reuse for outbound connections in an Arc<RwLock<HashSet>>. They are in turn unregistered when TcpListenStreams encounter fatal errors or are dropped.
A TcpListenStream uses an IfWatcher only if its listen socket is listening on all interfaces, analogously to the previous use of get_if_addrs in this case, with the advantage that the TcpListenStream is aware of expired addresses immediately. If used, the IfWatcher is lazily initialised when polling the TcpListenStream for events. Therefore TcpConfig::new() need not be async, reducing the changes to the public API and making for a smaller diff.
<TcpConfig as Transport>::address_translation(listen, observed) now returns Some(observed) when port reuse is enabled instead of None, as we want the observed address to be advertised to other peers as an "external address" as-is. I believe that returning None as was done in Tcp #1845 is an oversight, but please correct me if I'm mistaken.
More elaborate documentation, especially for the new TcpConfig::port_reuse() option as well as small test additions.

To avoid spawning a background task and thread within `TcpConfig::new()`, with communication via unbounded channels, a `TcpConfig` now keeps track of the listening addresses for port reuse in an `Arc<RwLock>`. Furthermore, an `IfWatcher` is only used by a `TcpListenStream` if it listens on any interface and directly polls the `IfWatcher` both for initialisation and new events. Includes some documentation and test enhancements.

transports/tcp/src/lib.rs

dvc94ch · 2020-12-14T14:29:51Z

Looks good to me, I like the changes.

quininer · 2020-12-15T09:50:30Z

Use async-io instead of macro-based code generation to support both tokio and async-std with less boilerplate and for easier code maintenance

I think async-io does not support tokio, please don't do it.

async-io uses its own background thread and global reactor and a C dependency, it cannot replace tokio.

mxinden

The changes look good to me. Thank you for the elaborate documentation.

Windows and Mac support

I have only tested this on Linux, not Mac nor Windows. Did you had a chance to test either of these platforms?

Tokio support

This pull request removes the tokio based chat example. I would suggest either:

Resurrecting the tokio chat example, using a tokio runtime to poll the async-io based TCP stack.
Introduce a test polling the async-io TCP stack with a tokio runtime.

I would prefer the latter. Having an automated way to ensure compatibility with async-std and tokio will make maintaining this stack a lot easier. This also ties into the concerns raised above by @quininer.

transports/tcp/src/lib.rs

romanb · 2020-12-15T11:37:24Z

async-io uses its own background thread and global reactor and a C dependency, it cannot replace tokio.

It is not intended to replace tokio. We are aware that there will be an extra async-io reactor thread when using tokio with libp2p-tcp to wake tasks waiting for TCP I/O, see also the changelog for the analogous change to libp2p-mdns. All tasks (i.e. futures), however, should still be polled by tokio if you use tokio. Other than the inconvenience of having an extra async-io reactor thread running besides tokio, what problems do you see? Is there something you think will not work correctly or very inefficiently? We need to find an alternative to feature flags and macros for the async-std vs tokio problem for libp2p crates that need either one. async-io currently seems like a good compromise, short of completely dropping support for one or the other. However, if someone wants to take another look at the options for abstracting over both in libp2p-mdns and libp2p-tcp without async-io or macros, that would be welcome, though that would also mean removing the use of if-watch again, because that uses async-io as well.

romanb · 2020-12-15T11:37:58Z

This pull request removes the tokio based chat example.

I actually intended to re-add it but forgot about it, thanks for the reminder.

quininer · 2020-12-15T12:05:08Z

It is not intended to replace tokio.

It does replace tokio, it replaces tokio's reactor.

see also the changelog for the analogous change to libp2p-mdns

I don't use mdns, sorry, I didn't pay attention to it.

what problems do you see? Is there something you think will not work correctly or very inefficiently?

Yes, I want to use tokio's reactor. I think the quality of mio is better than polling.

async-io currently seems like a good compromise, short of completely dropping support for one or the other.

I don't think that giving up support is a compromise. But I can understand the difficulty of maintaining both codes, and I will try to provide an abstraction layer.

romanb · 2020-12-15T12:41:47Z

Yes, I want to use tokio's reactor. I think the quality of mio is better than polling. [..] But I can understand the difficulty of maintaining both codes, and I will try to provide an abstraction layer.

Ok, I hope we can find a solution that is acceptable to both tokio and async-std users. It was our hope that using async-io would be an acceptable compromise. Do you intend to work on (a fork of) this branch or a separate crate? It is worth noting that, as far as I'm concerned, there is no rush. If we can get this branch to a satisfactory state early next year, that would be fine with me.

quininer · 2020-12-15T12:47:23Z

I plan to work in a separate crate, and I will open a PR when finished. :)

It was our hope that using async-io would be an acceptable compromise.

This is unacceptable to me, and I will not update the libp2p version until problem is resolved.

dvc94ch · 2020-12-15T13:15:23Z

async-io uses its own background thread and global reactor and a C dependency, it cannot replace tokio.

Yes it uses a C dependency on windows. But conceptionally this is a large step forward, completely separating the reactor from the executor. tokio is welcome to provide it's own reactor independent of the executor, and maybe we can use that instead.

Yes, I want to use tokio's reactor. I think the quality of mio is better than polling

Can you elaborate? I am interested in fixing any issues, but it seems a bit like a baseless claim.

I think async-io does not support tokio, please don't do it.

That is the advantage of async-io, it is completely independent of the executor.

I plan to work in a separate crate, and I will open a PR when finished.

Can you explain what solution you have in mind?

Interestingly @djc also does not find it a good solution on the grounds that "the ecosystem split is overblown as most production users use tokio" and @Darksonn also expressed that this "isn't a solution" suggesting generics instead. Sorry for paraphrasing or if I misrepresented opinions.

Darksonn · 2020-12-15T13:31:51Z

I'm not sure where exactly you are paraphrasing me from, but it does sound like something I would say. Generally I do not think that code that always uses a specific reactor is executor independent.

That said, I'm not surprised that you find using macros to write Tokio and async-std versions of the library cumbersome, and I definitely support doing something else. It is almost always possible to write the code using generics rather than macros, and if this is something that you are interested in, I would be happy to point you in the right direction.

Note that you do not have to expose a generic interface to the user. For example, hyper provides a non-generic interface using Tokio by default, but you can swap it out via an API based on generics. You could choose to provide non-generic versions with Tokio/async-std's types hard-coded together with an option to use the generic API directly for users that are on a third runtime.

quininer · 2020-12-15T13:41:20Z

Yes it uses a C dependency on windows. But conceptionally this is a large step forward

I don't think C dependency is a large step forward.

Can you elaborate? I am interested in fixing any issues, but it seems a bit like a baseless claim.

I will not respond to it, this is off topic.

That is the advantage of async-io, it is completely independent of the executor.

No one is talking about executor, I only focus on reactor.

Can you explain what solution you have in mind?

There are many solutions, the simplest is to use features-gate to abstract a minimal subset.

A better solution is to use factory generics, which will allow users to customize any tcp stream, not just async-io or tokio.

dvc94ch · 2020-12-15T13:53:57Z

I don't think C dependency is a large step forward.

I assume you are aware that compiling anything on windows requires a c toolchain. And mio uses miow written by @yoshuawuts one of the main async-std contributors. I really wouldn't want you to have to use such low quality code.

quininer · 2020-12-15T14:01:11Z

@dvc94ch I don't want to discuss digressions with you, please let us focus on how to solve the problem.

miow was transferred to @yoshuawuyts at my suggestion, if you know. see yoshuawuyts/miow#23 (comment)

dvc94ch · 2020-12-15T14:32:35Z

I plan to work in a separate crate, and I will open a PR when finished.

@quininer thank you for offering to fix this problem, looking forward to you solution. I guess you can get started on if-watch and open a PR. In the interest of not blocking this PR indefeninitely I'd ask you to commit to a timeline. Is 10d until the 25.12.20 enough time for you?

quininer · 2020-12-15T14:38:21Z

I do not intend and have no ability to block the PR, I just put forward my opinion, the PR can be merged as it is.

romanb · 2020-12-15T15:09:22Z

Again thanks for all the comments. I think for the rust-libp2p maintainers it is as simple as this: If there are tokio users who do take offence with some crates using async-io and hence a second reactor, we need to find another solution. As I said earlier, the hope was that async-io is an acceptable inconvenience for tokio users, but apparently it is more than that.

Regarding the future of this PR, I personally would only be merging it before we find a better abstraction if I can at least temporarily get if-watch and async-io behind an async-std feature, reintroducing the tokio feature otherwise for libp2p-tcp, ideally without the use of large macros. I don't think there would be a point in including this PR as-is in a release only to make substantial changes to the public API again soon after, so we may as well keep the feature flags for now and see if we can improve on or remove these macros some other way, which I may look into. In any case, looking forward to all other suggestions that will be put forward over time.

dvc94ch · 2020-12-15T15:45:46Z

~~Are we also going to add a tokio-v2 feature?~~ It seems there are projects that intending to wait on tokio 1.0, if I understand correctly hyper is one of them. This PR allowed actyx to update libp2p without a large inconvenience (which is also using tokio).

I'd suggest keeping the tcp feature flag and add a tcp-tokio3 feature flag. This might work for everyone.

dvc94ch · 2020-12-16T11:41:09Z

@romanb any chance we can get this released before christmas?

romanb · 2020-12-16T12:38:38Z

any chance we can get this released before christmas?

Unlikely, since I think many people will be on vacation next week or earlier, including myself, so that proper review of further changes is not really possible. I will update this PR as soon as I have a new proposal for keeping async-io and tokio separate in the manner of the code in master while incorporating the new features, but without excessive macros or code duplication.

To avoid having an extra reactor thread running for tokio users and to make sure all TCP I/O uses the mio-based tokio reactor. Thereby run tests with both backends.

romanb · 2021-01-04T12:45:43Z

With the recent commits I reintroduced feature flags for async-io (and thus if-watch) and tokio. I extracted a minimal interface as libp2p_tcp::provider::Provider for supporting both async-io and tokio without code duplication or excessive macros. This is a private interface for now, as it just gets the job done and is not intended to be polished for public exposure and custom implementations, which may eventually be nice to do if there is demand for it. The current implementations reside in libp2p_tcp::provider::async_io and libp2p_tcp::provider::tokio. In the context of the latest work I also made the tests of libp2p-tcp run with both async-io and tokio, so this PR now also closes #1820.

What is still missing from this PR is the resurrection of the tokio chat example, which I intend to do early next week before this PR may be merged and released, but I wanted to make the latest changes already visible now for further review.

quininer · 2021-01-04T13:55:34Z

This looks great! I made this, but I haven't opened the PR yet.

mxinden

I like the recent additions. I find the Provider abstraction with the two zero-variant enums neat. I will take another look once the example is in place. Thank you for putting more time and thoughts into this.

I am happy to wait some more in case @quininer would like to suggest additional changes.

transports/tcp/src/lib.rs

dvc94ch · 2021-01-05T12:44:20Z

Well, at this point you defaced the implementation. Looks like libp2p-rs just achieved feature parity with rust-libp2p.

[0] https://github.com/netwarps/libp2p-rs

romanb · 2021-01-05T14:55:08Z

Well, at this point you defaced the implementation.

I'm not sure I follow. The only difference of the current version to the one you liked is that all async-io and if-watch-specific calls are behind a simple interface, so that we can retain tokio support in the same way as it currently exists in master.

Looks like libp2p-rs just achieved feature parity with rust-libp2p.

That is great, especially if it is more to your liking, since it seems to specialise for async-std. It only needs to retain the Parity Technologies copyright notices alongside those of Netwarps Ltd, as is the minimal (and only) requirement of the MIT license.

Co-authored-by: Max Inden <mail@max-inden.de>

quininer · 2021-01-05T15:03:19Z

transports/tcp/Cargo.toml

-tokio = { version = "0.3", default-features = false, features = ["net"], optional = true }
+log = "0.4.11"
+socket2 = { version = "0.3.17", features = ["reuseport"] }
+tokio-crate = { package = "tokio", version = "0.3", default-features = false, features = ["net"], optional = true }


Maybe we can use tokio 1.0?

I would suggest we do the upgrade in a separate pull request, see #1907 and #1904.

transports/tcp/src/provider/tokio.rs

romanb · 2021-01-11T12:46:29Z

The chat-tokio example has been restored in 825cdc7. @mxinden if you could give this another review, that would be great.

dvc94ch and others added 6 commits December 3, 2020 17:42

Update tomls.

4ec695d

Let transports decide when to translate.

b571413

Improve tcp transport.

6f4603f

Update stuff.

65331c2

Merge branch 'master' into tcp

38f63f8

romanb mentioned this pull request Dec 14, 2020

Tcp #1845

Closed

dvc94ch reviewed Dec 14, 2020

View reviewed changes

transports/tcp/src/lib.rs Outdated Show resolved Hide resolved

transports/tcp/src/lib.rs Outdated Show resolved Hide resolved

mxinden reviewed Dec 15, 2020

View reviewed changes

transports/tcp/src/lib.rs Outdated Show resolved Hide resolved

transports/tcp/src/lib.rs Outdated Show resolved Hide resolved

romanb linked an issue Dec 15, 2020 that may be closed by this pull request

TCP transport: bind to the same port for outgoing connections #1563

Closed

Merge branch 'master' into tcp

878df1c

Reintroduce feature flags for tokio vs async-io.

448c350

To avoid having an extra reactor thread running for tokio users and to make sure all TCP I/O uses the mio-based tokio reactor. Thereby run tests with both backends.

romanb force-pushed the tcp branch from cd271d5 to 448c350 Compare January 4, 2021 12:17

Roman S. Borschel added 3 commits January 4, 2021 13:18

Merge branch 'master' into tcp

9e73b15

Add missing files.

2783a52

Fix docsrs attributes.

5ff8888

romanb linked an issue Jan 4, 2021 that may be closed by this pull request

transports/tcp: Unit test tokio feature #1820

Closed

mxinden reviewed Jan 5, 2021

View reviewed changes

transports/tcp/src/lib.rs Outdated Show resolved Hide resolved

transports/tcp/src/lib.rs Show resolved Hide resolved

Update transports/tcp/src/lib.rs

4479b66

Co-authored-by: Max Inden <mail@max-inden.de>

quininer reviewed Jan 5, 2021

View reviewed changes

Roman S. Borschel and others added 4 commits January 11, 2021 13:35

Restore chat-tokio example.

825cdc7

Merge branch 'tcp' of github.com:romanb/rust-libp2p into tcp

122f437

Forward poll_write_vectored for tokio's AsyncWrite.

07fd1fd

Merge branch 'master' into tcp

4235f19

mxinden approved these changes Jan 11, 2021

View reviewed changes

mxinden mentioned this pull request Jan 12, 2021

protocols/gossipsub: remove RPC message size check #1910

Closed

Roman S. Borschel added 3 commits January 12, 2021 11:31

Merge branch 'master' into tcp

da6ec2b

Merge branch 'master' into tcp

576077a

Update changelogs.

eb041ce

romanb merged commit ec0f8a3 into libp2p:master Jan 12, 2021

romanb deleted the tcp branch January 12, 2021 12:35

mxinden mentioned this pull request Jan 13, 2021

*: Update to libp2p v0.34.0 paritytech/substrate#7888

Merged

romanb mentioned this pull request Feb 23, 2021

Use https://crates.io/crates/p2p for NAT traversal? #348

Closed

elenaf9 mentioned this pull request Jul 10, 2022

transports/quic: Add implementation based on quinn-proto #2289

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[tcp] Port-reuse, async-io, if-watch #1887

[tcp] Port-reuse, async-io, if-watch #1887

romanb commented Dec 14, 2020

dvc94ch commented Dec 14, 2020

quininer commented Dec 15, 2020 •

edited

Loading

mxinden left a comment •

edited

Loading

romanb commented Dec 15, 2020

romanb commented Dec 15, 2020

quininer commented Dec 15, 2020 •

edited

Loading

romanb commented Dec 15, 2020

quininer commented Dec 15, 2020

dvc94ch commented Dec 15, 2020 •

edited

Loading

Darksonn commented Dec 15, 2020 •

edited

Loading

quininer commented Dec 15, 2020

dvc94ch commented Dec 15, 2020

quininer commented Dec 15, 2020

dvc94ch commented Dec 15, 2020 •

edited

Loading

quininer commented Dec 15, 2020

romanb commented Dec 15, 2020 •

edited

Loading

dvc94ch commented Dec 15, 2020 •

edited

Loading

dvc94ch commented Dec 16, 2020

romanb commented Dec 16, 2020

romanb commented Jan 4, 2021 •

edited

Loading

quininer commented Jan 4, 2021

mxinden left a comment

dvc94ch commented Jan 5, 2021

romanb commented Jan 5, 2021

quininer Jan 5, 2021

mxinden Jan 5, 2021

romanb commented Jan 11, 2021

[tcp] Port-reuse, async-io, if-watch #1887

[tcp] Port-reuse, async-io, if-watch #1887

Conversation

romanb commented Dec 14, 2020

dvc94ch commented Dec 14, 2020

quininer commented Dec 15, 2020 • edited Loading

mxinden left a comment • edited Loading

Choose a reason for hiding this comment

Windows and Mac support

Tokio support

romanb commented Dec 15, 2020

romanb commented Dec 15, 2020

quininer commented Dec 15, 2020 • edited Loading

romanb commented Dec 15, 2020

quininer commented Dec 15, 2020

dvc94ch commented Dec 15, 2020 • edited Loading

Darksonn commented Dec 15, 2020 • edited Loading

quininer commented Dec 15, 2020

dvc94ch commented Dec 15, 2020

quininer commented Dec 15, 2020

dvc94ch commented Dec 15, 2020 • edited Loading

quininer commented Dec 15, 2020

romanb commented Dec 15, 2020 • edited Loading

dvc94ch commented Dec 15, 2020 • edited Loading

dvc94ch commented Dec 16, 2020

romanb commented Dec 16, 2020

romanb commented Jan 4, 2021 • edited Loading

quininer commented Jan 4, 2021

mxinden left a comment

Choose a reason for hiding this comment

dvc94ch commented Jan 5, 2021

romanb commented Jan 5, 2021

quininer Jan 5, 2021

Choose a reason for hiding this comment

mxinden Jan 5, 2021

Choose a reason for hiding this comment

romanb commented Jan 11, 2021

quininer commented Dec 15, 2020 •

edited

Loading

mxinden left a comment •

edited

Loading

quininer commented Dec 15, 2020 •

edited

Loading

dvc94ch commented Dec 15, 2020 •

edited

Loading

Darksonn commented Dec 15, 2020 •

edited

Loading

dvc94ch commented Dec 15, 2020 •

edited

Loading

romanb commented Dec 15, 2020 •

edited

Loading

dvc94ch commented Dec 15, 2020 •

edited

Loading

romanb commented Jan 4, 2021 •

edited

Loading