Don't panic in libp2p-websocket

[hrxi]

Return an error instead. quicksink panics if you call a method after
it returned an error once.

Fixes #2598.

[mxinden]

Do I understand correctly that something within the stack is calling SinkImpl::poll_ready even though the previous call returned an error? If so, instead of silencing the error as suggested here, have you tried locating the logic that is responsible for this second try @hrxi?

[hrxi]

Do I understand correctly that something within the stack is calling SinkImpl::poll_ready even though the previous call returned an error?

This is correct.

If so, instead of silencing the error as suggested here, have you tried locating the logic that is responsible for this second try @hrxi?

The pull request does not silence an error, it will return with an error status once any error has been observed.

To me, this seems like a strict improvement over the panic semantics, in fact the authors of quicksink also approved a pull request with this behavior: paritytech/quicksink#4. The pull request was reverted due to unrelated API concerns which don't apply to this change: paritytech/quicksink#6.

The original pull request also included a usage scenario, though I haven't evaluated its usefulness:

This however makes it more difficult to use SinkImpl in certain contexts, e.g. concurrently using locks, when for instance one thread successfully closes the sink using poll_close and another one calls poll_ready afterwards (Thanks @romanb for this example).

[mxinden]

have you tried locating the logic that is responsible for this second try

I would expect other Sink implementations to have similar behaviors, i.e. panic as well, thus I guess it is worth locating the component that retried after an error.

In which setup are you using the Connection @hrxi? Are you upgrading the Connection with a stream muxer?

[hrxi]

I would expect other Sink implementations to have similar behaviors, i.e. panic as well, thus I guess it is worth locating the component that retried after an error.

I don't expect other Sink implementation to have similar behavior. Do you know of any? There was a quicksink issue about it and even the author of quicksink agreed that it's bad to panic there, but due to API concerns couldn't change it.

[jasl]

Just a friendly ping, any progress here? I heard other parachain teams also met this issue, don't know why polkadot-v0.9.23 branch trigger very frequently

[bkchr]

@mxinden can we get this merged? Or should we open a pr with a better solution?

[tomaka]

As the conversation says, this PR is merely hiding the actual bug instead of fixing it, and it is IMO the wrong approach. If you want to help, you can help try to find the actual bug rather than pressuring to merge a PR.

[mxinden]

Or should we open a pr with a better solution?

That would be very much appreciated.

Referencing @tomaka's suspicion: #2598 (comment)

[bkchr]

Okay perfect. @kpp can you please look into @tomaka's suspicion?

[mxinden]

Fixed with yamux v0.10.2. See libp2p/rust-yamux#138 (comment)

I am closing here. Thanks for the hep everyone!