protocols/gossipsub: Move to stable futures #22
Conversation
|
Sounds good. |
|
Hey max. If you have the time, could you target this PR to the |
|
@AgeManning I am currently reworking this branch, given that quite a bit changed on the base branch since I opened it. Will force-push to this branch, given that there are no reviews on this pull request. Will keep you updated. |
mxinden
force-pushed
the
gossipsub-stable-futures
branch
from
2f46275 to
3229a5c
Compare
mxinden
force-pushed
the
gossipsub-stable-futures
branch
from
3229a5c to
542b100
Compare
|
@AgeManning please take a look. |
protocols/gossipsub/src/handler.rs
Outdated
| return Poll::Ready(ProtocolsHandlerEvent::Custom(message)); | ||
| } | ||
| Poll::Ready(Some(Err(e))) => { | ||
| // TODO: Should we really just close here? Not even log the error? |
There was a problem hiding this comment.
I think we should log the error, then close. This should potentially even be logged as an Err, as we probably should destroy the connection.
I had originally written this for interoperability with go and really didn't want to terminate connections. So the current version keeps running as long as we have one active (inbound or outbound) stream with the peer. If we lose the inbound stream, we probably should close the connection. What do you think?
There was a problem hiding this comment.
If we lose the inbound stream, we probably should close the connection.
Are you suggesting to set self.keep_alive to false regardless of self.outboundstream being None or Some?
There was a problem hiding this comment.
So we close the connection when closing an inbound stream fails, but we don't close the connection when receiving an error on an inbound substream when polling unless it will later on fail during closing of the inbound stream. That seems a bit strange, no?
There was a problem hiding this comment.
I'm saying we should handle the error cases symmetrically for inbound/outbound substreams. On errors we can return ProtocolsHandlerEvent::close() or try to close the substream and return the error if closing the substream errors.
Currently, I have it that when we close one of them, the connection remains open unless both are closed. This favours connectivity on errors (if we can successfully close after an error). Maybe this makes it more resilient and we keep this logic?
protocols/gossipsub/src/handler.rs
Outdated
| Err(_) => { | ||
| return Err(io::Error::new( | ||
| Poll::Ready(Err(_)) => { | ||
| // TODO: Why do we not at least log the error (`_`)? |
There was a problem hiding this comment.
Yeah I think we should log the error. I think the first time I wrote this I was trying to match the go-impl so was in a hurry.
protocols/gossipsub/src/handler.rs
Outdated
| Err(_) => { | ||
| return Err(io::Error::new( | ||
| Poll::Ready(Err(_)) => { | ||
| // TODO: Why do we not at least log the error (`_`)? |
There was a problem hiding this comment.
Yeah I think we should log the error. I think the first time I wrote this I was trying to match the go-impl so was in a hurry.
protocols/gossipsub/src/handler.rs
Outdated
| return Poll::Ready(ProtocolsHandlerEvent::Custom(message)); | ||
| } | ||
| Poll::Ready(Some(Err(e))) => { | ||
| // TODO: Should we really just close here? Not even log the error? |
There was a problem hiding this comment.
I think we should log the error, then close. This should potentially even be logged as an Err, as we probably should destroy the connection.
I had originally written this for interoperability with go and really didn't want to terminate connections. So the current version keeps running as long as we have one active (inbound or outbound) stream with the peer. If we lose the inbound stream, we probably should close the connection. What do you think?
|
@AgeManning as far as I can tell all comments are addressed. In regards to the discussion when to close a connection:
That sounds good to me. When only closing a connection on outbound errors,we might end up with a bunch of unidirectional connections. I would defer whatever change we want to make to another pull request, as it is out of scope of stable-futures. What do you think? |
|
Sounds good to me. |
This moves the
protocols/gossipsubcrate to stable futures along the lines of libp2p#1328.Let's get #21 in first and then follow up with this pull request.
Only the last 4 commits are relevant, the remainder is from merging libp2p#1328.