Report aborted connection from Pool::poll #2369

jmmaloney4 · 2021-12-07T21:29:50Z

First attempt at fixing #2329.

MarcoPolo

Looks good!

Bonus points if you add a test case to this :)
Maybe something like:

create new pool
call pool.add_outgoing(...) with a transport (see examples)
pool.disconnect
pool.pool and see if you see a PoolEvent::PendingOutboundConnectionError
(I haven't tried this, so it may not work)

MarcoPolo · 2021-12-08T00:42:45Z

core/src/connection/pool.rs

@@ -50,7 +50,8 @@ use std::{
    task::Context,
    task::Poll,
 };
-use void::Void;
+
+use self::task::PendingConnectionCommand;


nit, I would not use ... this directly and instead follow the same pattern as EstablishedConnectionCommand referencing it by the task namespace. E.g. task::PendingConnectionCommand

jmmaloney4 · 2021-12-17T06:32:01Z

I tried adding a test case, but I'm not sure how to call the Pool::poll method without an async context. What would be the right way to do that in a test case?

MarcoPolo · 2021-12-18T02:31:13Z

Nice work @jmmaloney4 I filled in some of the missing pieces here: 0c61938.

I had to move the test to be within the module since the pool module is pub(crate) (so not usable from an external crate like those test).

Feel free to cherry pick that commit :)

jmmaloney4 · 2022-01-06T22:58:01Z

Thanks! I took your code and moved it into my branch, and I also tried to make the TestHandler implementations shared, but I'm not sure if I did that in an appropriate way--I had to make the util module pub here:

rust-libp2p/core/src/connection.rs

Line 25 in a3ff8e2

pub mod util;

…Handler

thomaseizinger

Thanks!

Left some comments inline.

thomaseizinger · 2022-01-07T03:03:13Z

core/src/connection/pool/task.rs

        Either::Left((Err(oneshot::Canceled), _)) => {
+            unreachable!("Pool never drops channel to task.");


This is not quite true. It is dropped immediately after abort finishes because we are consuming self in there.

I've read through #2329 but I don't quite understand why we need to send a dedicated value now and the drop notifier isn't enough?

We always initialize abort_receiver with Some so there is never a case where we call abort and wouldn't be able to send PendingConnectionCommand::Abort.

We are still dropping the entire struct at the end of abort so all the data is gone anyway.

We are not sending any information along with PendingConnectionCommand::Abort.

Unless I am missing something, reacting to Canceled should be equivalent to what we currently have here? The abort method can just consume self and do nothing with it which will immediately drop it and trigger the drop notifier. That will still be the same public API albeit it may be worth considering removing abort and using std::mem::drop instead if we want to be explicit about the dropping.

This is not quite true. It is dropped immediately after abort finishes because we are consuming self in there.

Maybe there's a misunderstanding. The thing that gets dropped is PendingConnection, the thing that has the abort_notifier is PendingConnectionInfo. The PendingConnectionInfo stays around in the hash map of the pool.

Whether you actually need to send abort vs just taking the notfier and dropping it. I think you can do the same thing by dropping the notifier. The reason you may want the abort value here is to make the distinction between this was dropped and we aborted this. Although with the unreachable we're saying we would never drop this (I'm not fully sure that's true, I need to look more closely).

This is not quite true. It is dropped immediately after abort finishes because we are consuming self in there.

Maybe there's a misunderstanding. The thing that gets dropped is PendingConnection, the thing that has the abort_notifier is PendingConnectionInfo. The PendingConnectionInfo stays around in the hash map of the pool.

Ah what a subtle difference, I missed that, thank you!

The reason you may want the abort value here is to make the distinction between this was dropped and we aborted this. Although with the unreachable we're saying we would never drop this (I'm not fully sure that's true, I need to look more closely).

That is what my closer look was triggered by! We are adding a way of distinguishing the two and then asserting that one of them never happens. So why add that in the first place?

It seems to be sufficient to change the oneshot::Sender<Void> to an Option<oneshot::Sender<Void>> so we can take and mem::drop it which will trigger the cancellation. That should avoid the unreachable within the select.

thomaseizinger · 2022-01-07T03:15:35Z

core/src/connection/util.rs

+
+
+#[derive(Debug)]
+pub struct TestHandler();


Instead of making this handler public, can we move the test you added to tests/ and hence reuse the handler from there? I don't think we should be adding this one to the public about of libp2p-core.

To resolve the issue about Pool being pub(crate), I'd suggest to write the unit test against the Network component (which is the public API of libp2p-core).

The structure should roughly be the same:

Make a new Network

dial a peer

Retrieve it with peer

Convert it into a DialingPeer

disconnect the DialingPeer

Observe the aborted event

Oh! good idea, thanks!

thomaseizinger · 2022-01-07T03:16:32Z

core/src/connection/pool/task.rs

+
+/// Commands that can be sent to a task driving an established connection.
+#[derive(Debug)]
+pub enum EstablishedConnectionCommand<T> {


If my assumption above is correct, I'd vote for undoing this rename to reduce the noise of the diff.

…il::TestHandler" This reverts commit 40cdec3.

…ommand

MarcoPolo · 2022-01-10T19:27:17Z

@jmmaloney4 added the suggestions from @thomaseizinger in my branch here: https://github.com/libp2p/rust-libp2p/compare/master...MarcoPolo:marco/fix-2329-v2?expand=1 (I can't push to this branch). Feel free to cherry pick those commits here.

mxinden · 2022-02-01T19:23:27Z

Friendly ping @jmmaloney4.

mxinden · 2022-02-14T21:18:11Z

There have been some larger changes (#2492). To make things easier on your end I resolved the conflicts. As far as I can there is nothing else missing, thus I am closing here with the goal of merging via #2517.

Please leave any comments on #2517 in case I am missing something.

Thanks @jmmaloney4 @MarcoPolo and @thomaseizinger for the work!

MarcoPolo reviewed Dec 8, 2021

View reviewed changes

jmmaloney4 added 3 commits December 10, 2021 20:48

Report aborted connection from Pool::poll

7173d33

Don't use ... task::PendingConnectionCommand

48723f0

First attempt on testing aborted connections

16188bb

Fix tests and move tests::util::TestHandler to connection::util::Test…

40cdec3

…Handler

thomaseizinger requested changes Jan 7, 2022

View reviewed changes

MarcoPolo added 4 commits January 10, 2022 11:10

Revert "Fix tests and move tests::util::TestHandler to connection::ut…

d695ca6

…il::TestHandler" This reverts commit 40cdec3.

Add debug msg

076b486

Add test using public network object

1ffa811

Signal abort by closing channel. And s/EstablishedConnectionCommand/C…

549861c

…ommand

mxinden mentioned this pull request Feb 14, 2022

swarm/: Report aborted connections #2517

Merged

mxinden closed this Feb 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Report aborted connection from Pool::poll #2369

Report aborted connection from Pool::poll #2369

jmmaloney4 commented Dec 7, 2021

MarcoPolo left a comment

MarcoPolo Dec 8, 2021

jmmaloney4 commented Dec 17, 2021

MarcoPolo commented Dec 18, 2021

jmmaloney4 commented Jan 6, 2022

thomaseizinger left a comment

thomaseizinger Jan 7, 2022

MarcoPolo Jan 8, 2022

thomaseizinger Jan 9, 2022

thomaseizinger Jan 7, 2022

thomaseizinger Jan 7, 2022

MarcoPolo Jan 8, 2022

thomaseizinger Jan 7, 2022

MarcoPolo commented Jan 10, 2022

mxinden commented Feb 1, 2022

mxinden commented Feb 14, 2022

		Either::Left((Err(oneshot::Canceled), _)) => {
		unreachable!("Pool never drops channel to task.");

Report aborted connection from Pool::poll #2369

Report aborted connection from Pool::poll #2369

Conversation

jmmaloney4 commented Dec 7, 2021

MarcoPolo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmmaloney4 commented Dec 17, 2021

MarcoPolo commented Dec 18, 2021

jmmaloney4 commented Jan 6, 2022

thomaseizinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoPolo commented Jan 10, 2022

mxinden commented Feb 1, 2022

mxinden commented Feb 14, 2022