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

feat(swarm): close connections on multistream-select protocol violations #3482

Closed
wants to merge 1 commit into from

Conversation

thomaseizinger
Copy link
Contributor

Description

As per discussion in #3353, this patch proposes to not report what I deem to be fatal errors during protocol negotiation to a ConnectionHandler but instead close the entire connection.

The error is returned to the Swarm as the reason for why the connection is being closed.

Notes

I consider this a medium-risk PR because it introduces a potentially drastic change in behaviour. Previously, a ConnectionHandler could simply ignore these errors and a peer that only happened to f.e. send a bad protocol name for one stream but was otherwise well-behaved worked fine. With this patch, such a connection would get closed by the Swarm.

Links to any relevant issues

Open Questions

  • Is closing the connection the right thing to do? We could differentiate between the various kinds of ProtocolError and only "close" the connection on IO errors but still report protocol violations back to the ConnectionHandler?

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

@mxinden
Copy link
Member

mxinden commented Feb 19, 2023

I am in favor of no longer forwarding errors to a ConnectionHandler that it has no way to properly handle. I will take an in-depth look. Thanks for the draft!

@mxinden
Copy link
Member

mxinden commented Feb 24, 2023

  • Is closing the connection the right thing to do?

Say a remote peer opens a stream, starts a multistream-select negotiation, but aborts half-way through and drops the stream, thus leading to an IO error on the stream on the side of the local node. Do I understand correctly that with this patch the whole connection would be closed? If so, I don't think that is a good behavior as a single failing stream does not imply that the whole connection is useless.

@thomaseizinger
Copy link
Contributor Author

  • Is closing the connection the right thing to do?

Say a remote peer opens a stream, starts a multistream-select negotiation, but aborts half-way through and drops the stream, thus leading to an IO error on the stream on the side of the local node. Do I understand correctly that with this patch the whole connection would be closed? If so, I don't think that is a good behavior as a single failing stream does not imply that the whole connection is useless.

True, we'd have to still report those back to the handler I guess.

@thomaseizinger
Copy link
Contributor Author

Closing this because we discovered unintended consequences (see discussion above).

@mxinden
Copy link
Member

mxinden commented Mar 13, 2023

We are in agreement that the IO error should not close the connection.

  • Is closing the connection the right thing to do?

Say a remote peer opens a stream, starts a multistream-select negotiation, but aborts half-way through and drops the stream, thus leading to an IO error on the stream on the side of the local node. Do I understand correctly that with this patch the whole connection would be closed? If so, I don't think that is a good behavior as a single failing stream does not imply that the whole connection is useless.

True, we'd have to still report those back to the handler I guess.

We are not in agreement here. I don't think an IO error during protocol negotiation of an inbound stream needs to be reported to the handler. There isn't even a specific handler to deliver the error to, as we don't yet know the protocol as negotiating it failed in the first place.

@thomaseizinger
Copy link
Contributor Author

  • Is closing the connection the right thing to do?

Say a remote peer opens a stream, starts a multistream-select negotiation, but aborts half-way through and drops the stream, thus leading to an IO error on the stream on the side of the local node. Do I understand correctly that with this patch the whole connection would be closed? If so, I don't think that is a good behavior as a single failing stream does not imply that the whole connection is useless.

True, we'd have to still report those back to the handler I guess.

We are not in agreement here. I don't think an IO error during protocol negotiation of an inbound stream needs to be reported to the handler. There isn't even a specific handler to deliver the error to, as we don't yet know the protocol as negotiating it failed in the first place.

Fair, I forgot about that. Let me investigate what that leads to.

@thomaseizinger
Copy link
Contributor Author

I opened a new PR: #3605

@thomaseizinger thomaseizinger deleted the feat/automatically-close-connection branch April 26, 2023 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants