feat(transport): add experimental QUIC Transport (not production ready) #725
Conversation
Co-authored-by: markspanbroek <mark@spanbroek.net>
|
Unfortunately, nim-quic relies on questionable which doesn't work on nim devel So even though this branch works, it should not be merged until this get fixed |
|
We should not be using Questionable and the likes, are totally fine in a high level application, where the amount of "busyness" related logic might become high (Codex, Nimbus, etc...), but not in a low level dependency such as |
# Conflicts: # .pinned # libp2p.nim # libp2p.nimble # libp2p/wire.nim # tests/testswitch.nim
diegomrsantos
changed the title
| _: type QuicStream, stream: Stream, oaddr: Opt[MultiAddress], peerId: PeerId | ||
| ): QuicStream = | ||
| let quicstream = QuicStream(stream: stream, observedAddr: oaddr, peerId: peerId) | ||
| procCall P2PConnection(quicstream).initStream() |
There was a problem hiding this comment.
You should specify an objName for quicstream before calling initStream. https://github.com/vacp2p/nim-libp2p/blob/master/libp2p/stream/connection.nim#L56-L57
| except QuicError: | ||
| raise newLPStreamEOFError() | ||
| except CatchableError: | ||
| raise newLPStreamEOFError() |
There was a problem hiding this comment.
After a recent discussion with @Ivansete-status, I think those errors need a small rework, they lack precision (not in this PR though). But nevertheless, everytime you use LPStreamEOFError you should try to use one of those error instead.
| # Session | ||
| type QuicSession* = ref object of P2PConnection | ||
| connection: QuicConnection |
There was a problem hiding this comment.
I'm not sure to understand why you need two P2PConnections here (with QuicSession and QuicStream). It's a bit confusing. QuicSession looks like ConnManager to me.
And I think it's better to create your custom Connection in libp2p/streams instead of here.
|
|
||
| # Stream | ||
| type QuicStream* = ref object of P2PConnection | ||
| stream: Stream |
There was a problem hiding this comment.
I'm not a huge fan of this un-prefixed Stream. Maybe renaming it QuicStream and renaming the current QuicStream into LPQuicStream will be clearer.
| handleFut: Future[void] | ||
|
|
||
| method newStream*( | ||
| m: QuicMuxer, name: string = "", lazy: bool = false |
There was a problem hiding this comment.
I don't think the lazy flag is an option here. Maybe add a log if someone tries to use it.
| try: | ||
| while not m.quicSession.atEof: | ||
| let incomingStream = await m.quicSession.getStream(Direction.In) | ||
| asyncSpawn m.handleStream(incomingStream) |
There was a problem hiding this comment.
If you find an alternative to the asyncspawn, it would be great.
| type QuicUpgrade = ref object of Upgrade | ||
|
|
||
| type QuicTransport* = ref object of Transport | ||
| listener: Listener |
There was a problem hiding this comment.
Same as above, not a fan of un-prefixed object like that. QuicListener is great.
There was a problem hiding this comment.
This is defined in nim-quic.
| QUIC_V1.match(address) | ||
|
|
||
| method start*(transport: QuicTransport, addrs: seq[MultiAddress]) {.async.} = | ||
| doAssert transport.listener.isNil, "start() already called" |
There was a problem hiding this comment.
Should this really be a doAssert? Raising an LPError should be a better idea.
| for c in transport.connections: | ||
| await c.close() |
There was a problem hiding this comment.
What about something like that: await allFutures(transport.connections.mapIt(it.close))
There was a problem hiding this comment.
Is there any semantic difference? Not sure if it adds enough value to justify the change.
| let connection = await dial(initTAddress(address).tryGet) | ||
| return transport.wrapConnection(connection) |
There was a problem hiding this comment.
You should probably check if transport.running == true
Our quic effort is blocked by bearssl not supporting TLS1.3, but since Mark did most of the work to implement Quic here: #563 and on nim-quic, this PR is going to bring encryption-less Quic into nim-libp2p
This allows us to test it, and make sure it doesn't bitrot.
Heavily WiP:
observed addressand port 0 resolving)Since encryption is not yet supported, we're not compatible with any other libp2ps, and have to rely on home made protocols to retrieve the peer's id