LibP2P network backends #278
Conversation
beacon_chain/libp2p_backend.nim
Outdated
| @@ -166,68 +136,55 @@ proc sendBytes(stream: P2PStream, bytes: Bytes) {.async.} = | |||
|
|
|||
| proc makeEth2Request(peer: Peer, protocolId: string, requestBytes: Bytes, | |||
| ResponseMsg: type, | |||
| timeout = 10000): Future[Option[ResponseMsg]] {.async.} = | |||
| timeout = 10.seconds): Future[Option[ResponseMsg]] {.async.} = | |||
| var stream = await peer.network.daemon.openStream(peer.id, @[protocolId]) | |||
| # TODO how does openStream fail? Set a timeout here and handle it | |||
| let sent = await stream.transp.write(requestBytes) | |||
| # TODO: Should I check that `sent` is equal to the desired number of bytes | |||
There was a problem hiding this comment.
You should check and if result != len(requestBytes), then your connection is dead, but still requires transport.close().
There was a problem hiding this comment.
If performing these steps is necessary every single time, why don't we introduce a simpler API where close is being called automatically on failure?
Are there situations where it won't be appropriate to call close immediately?
There was a problem hiding this comment.
Its actually very bad idea to make this simple API, because its very easy to make race condition or double close in such way. And when we fix rlpx.nim with @kdeme you will see that there will be just one close call in code.
There was a problem hiding this comment.
Can you provide an example of these race conditions, so I can understand the problem better?
Isn't it possible to make the API easier to use in general if double close wasn't a problem in the first place?
There was a problem hiding this comment.
In rlpx, the "transport already closed" exceptions do not happen because of a double close.
In fact, from what I understand, a double close will not give trouble and the API takes care of it: see https://github.com/status-im/nim-chronos/blob/a8a1138b8b66ac51378897ef126b3783dcf9a2ca/chronos/transports/stream.nim#L1750
In rlpx, the "transport already closed" exceptions happen when trying to read or write data on a transport which we already closed. This can (and niw is ?mostly?) being taken care of by just handling the exceptions. But it feels a bit unnecessary and expensive. Probably also less error prone if the read/writes just don't occur any more. I believe this is what @cheatfate means with the races.
beacon_chain/libp2p_backend.nim
Outdated
| include eth/p2p/p2p_tracing | ||
|
|
||
| import typetraits | ||
|
|
||
| proc readMsg(stream: P2PStream, MsgType: type, |
There was a problem hiding this comment.
This procedure do not have any error handlers.
beacon_chain/libp2p_backend.nim
Outdated
| proc readMsg(stream: P2PStream, MsgType: type, | ||
| timeout = 10000): Future[Option[MsgType]] {.async.} = | ||
| timeout = 10.seconds): Future[Option[MsgType]] {.async.} = |
There was a problem hiding this comment.
Timeout here is only used when prefix is received and not used for receiving body.
zah
force-pushed
the
libp2p-builds
branch
2 times, most recently
from
ba0c65f
to
0bc8e01
Compare
…ak the cyclic imports
…twork back-end type
You can switch between the available backends by compiling with
-d:network_type=<backend_name>.The default backend is now "libp2p_spec" which implements the spec published here:
https://github.com/ethereum/eth2.0-specs/tree/dev/specs/networking
A forward-looking backend called "libp2p_native" attempts to take advantage of the existing multi-plexing capabilities of LibP2P. Hopefully, we'll be able to switch to it once Multistream-2.0 lands and eliminates the currently present overhead when using a separate stream for each request.
The old RLPx backend is still available when compiling with
-d:network_type=rlpx.