fix(perf): add QUIC transport compatibility #1524
Conversation
MorganaFuture
force-pushed
the
MorganaFuture/fix/quic_does_not_work_with_perf_protocol
branch
from
715ff29 to
bb02e59
Compare
| when defined(libp2p_quic_support): | ||
| if not (conn of QuicStream): | ||
| await conn.close() | ||
| # For QUIC streams, don't close yet - let server manage lifecycle | ||
| else: | ||
| await conn.close() |
There was a problem hiding this comment.
this would be nicer if could be written like this
if not (conn of QuicStream):
await conn.close()
without when defined(libp2p_quic_support): because if it is quic conn then we can assume that quic is supported. and in other places...
There was a problem hiding this comment.
is when defined(libp2p_quic_support) necessary in these cases?
There was a problem hiding this comment.
@vladopajic
Thanks for your feedback! I considered simplifying the
when
defined(libp2p_quic_support)
blocks, but they're actually necessary for compile-time safety.
Without the conditional compilation, the code would fail to compile when QUIC support is disabled because:
QuicStreamtype wouldn't be declaredconn of QuicStreamwould cause a compilation error
Alternative approaches like string-based type checking would be less efficient and less type-safe than the
current compile-time approach.
The when defined pattern is commonly used throughout nim-libp2p for optional features to ensure the code compiles correctly regardless of which transports are enabled/disabled.
There was a problem hiding this comment.
thanks! we should also explore improving this. it would be much nicer code if we can write it without when defined(libp2p_quic_support): in examples like these. ideally library should only forbid to instantiate quic transport but types should be there for simplifying code
| # QUIC needs timeout-based approach to detect end of upload | ||
| while not conn.atEof: | ||
| let readFut = conn.readOnce(addr toReadBuffer[0], PerfSize) | ||
| if not await readFut.withTimeout(100.milliseconds): |
There was a problem hiding this comment.
if this is the case then we should probably fix that, as this can be vary flaky
There was a problem hiding this comment.
we can merge pr with this timeout, and open issue to to track effort on fixing this.
There was a problem hiding this comment.
@vladopajic Agreed, the timeout approach is indeed flaky.
Let's merge this PR with the current timeout implementation to fix the immediate QUIC compatibility issue.
Could you open a separate issue to track implementing a more robust EOF detection mechanism for QUIC streams? Feel free to assign it to me - I'll research proper QUIC stream lifecycle management and explore better alternatives to the timeout-based approach.
MorganaFuture
force-pushed
the
MorganaFuture/fix/quic_does_not_work_with_perf_protocol
branch
from
bb02e59 to
fdc840a
Compare
vladopajic
changed the title
|
@vladopajic Could you please rerun CI/CD? It looks like flaky error... |
|
@MorganaFuture I'm seeing this, is this what you're seeing? https://github.com/vacp2p/nim-libp2p/actions/runs/16267405031/job/45929192700?pr=1524 Could you please link to what you're seeing if different. I might be seeing some flakey tests on my end, and I'm on the lookout for other occurrences that can help me see a pattern emerge. Could you please describe the flakiness of it, and if there's anything on your end that might help understand what's making it flakey, that might help as well. |
QUIC doesn't support half-close semantics like TCP multiplexed transports. Perf client was closing connection after write but before read, breaking QUIC streams. - Move connection close after read phase for TCP transports - Let QUIC server manage connection lifecycle - Add timeout-based read detection for QUIC streams - Fix infinite read loop in TCP server handling - Make QUIC calls conditional on libp2p_quic_support
MorganaFuture
force-pushed
the
MorganaFuture/fix/quic_does_not_work_with_perf_protocol
branch
from
66082e9 to
6ea3079
Compare
|
CI is passing for this PR. The code coverage step fails, but it's not caused by changes in this PR. @richard-ramos can we force merge this PR? |
|
🚀 |
5 participants
What
Fixed QUIC transport compatibility with perf protocol.
Why
QUIC doesn't support half-close like TCP transports. Perf was closing connection
after write but before read, breaking QUIC streams with "stream is closed" errors.
Now both QUIC and TCP transports work correctly with perf protocol.
Links
Issue 1482