search

vacp2p / nim-libp2p

libp2p implementation in Nim
https://vacp2p.github.io/nim-libp2p/docs/
MIT License
240 stars 52 forks source link

fix(transport): various tcp transport races #1095

Closed arnetheduck closed 1 month ago

arnetheduck commented 1 month ago

This PR is more or less a rewrite of the TCP transport to fix several race conditions and leaks that can happen during startup / shutdown as well as spurious exceptions leaking out of it

diegomrsantos commented 1 month ago

What do you think about splitting this PR into smaller ones, perhaps one for each TCP proc? There are too many changes in the TCP transport file, GitHub doesn't even load them by default. I'd argue it's too risky to make all those modifications at once. If there's an issue, it might be difficult to understand the cause.

codecov-commenter commented 1 month ago

Codecov Report

Attention: Patch coverage is 84.69945% with 28 lines in your changes are missing coverage. Please review.

Project coverage is 84.51%. Comparing base (02c96fc) to head (1c65f04). Report is 4 commits behind head on master.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/vacp2p/nim-libp2p/pull/1095/graphs/tree.svg?width=650&height=150&src=pr&token=M88zHaQffJ&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vacp2p)](https://app.codecov.io/gh/vacp2p/nim-libp2p/pull/1095?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vacp2p) ```diff @@ Coverage Diff @@ ## master #1095 +/- ## ========================================== - Coverage 84.53% 84.51% -0.03% ========================================== Files 91 91 Lines 15517 15534 +17 ========================================== + Hits 13118 13129 +11 - Misses 2399 2405 +6 ``` | [Files](https://app.codecov.io/gh/vacp2p/nim-libp2p/pull/1095?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vacp2p) | Coverage Δ | | |---|---|---| | [libp2p/dialer.nim](https://app.codecov.io/gh/vacp2p/nim-libp2p/pull/1095?src=pr&el=tree&filepath=libp2p%2Fdialer.nim&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vacp2p#diff-bGlicDJwL2RpYWxlci5uaW0=) | `91.56% <100.00%> (+0.84%)` | :arrow_up: | | [libp2p/errors.nim](https://app.codecov.io/gh/vacp2p/nim-libp2p/pull/1095?src=pr&el=tree&filepath=libp2p%2Ferrors.nim&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vacp2p#diff-bGlicDJwL2Vycm9ycy5uaW0=) | `66.66% <ø> (ø)` | | | [libp2p/switch.nim](https://app.codecov.io/gh/vacp2p/nim-libp2p/pull/1095?src=pr&el=tree&filepath=libp2p%2Fswitch.nim&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vacp2p#diff-bGlicDJwL3N3aXRjaC5uaW0=) | `90.52% <100.00%> (-1.19%)` | :arrow_down: | | [libp2p/transports/tortransport.nim](https://app.codecov.io/gh/vacp2p/nim-libp2p/pull/1095?src=pr&el=tree&filepath=libp2p%2Ftransports%2Ftortransport.nim&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vacp2p#diff-bGlicDJwL3RyYW5zcG9ydHMvdG9ydHJhbnNwb3J0Lm5pbQ==) | `83.23% <100.00%> (-0.10%)` | :arrow_down: | | [libp2p/transports/transport.nim](https://app.codecov.io/gh/vacp2p/nim-libp2p/pull/1095?src=pr&el=tree&filepath=libp2p%2Ftransports%2Ftransport.nim&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vacp2p#diff-bGlicDJwL3RyYW5zcG9ydHMvdHJhbnNwb3J0Lm5pbQ==) | `92.30% <100.00%> (+3.84%)` | :arrow_up: | | [libp2p/wire.nim](https://app.codecov.io/gh/vacp2p/nim-libp2p/pull/1095?src=pr&el=tree&filepath=libp2p%2Fwire.nim&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vacp2p#diff-bGlicDJwL3dpcmUubmlt) | `62.66% <66.66%> (+0.76%)` | :arrow_up: | | [libp2p/transports/tcptransport.nim](https://app.codecov.io/gh/vacp2p/nim-libp2p/pull/1095?src=pr&el=tree&filepath=libp2p%2Ftransports%2Ftcptransport.nim&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vacp2p#diff-bGlicDJwL3RyYW5zcG9ydHMvdGNwdHJhbnNwb3J0Lm5pbQ==) | `85.27% <84.02%> (+4.70%)` | :arrow_up: | ... and [7 files with indirect coverage changes](https://app.codecov.io/gh/vacp2p/nim-libp2p/pull/1095/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vacp2p)
diegomrsantos commented 1 month ago

This is flaky on mac https://github.com/vacp2p/nim-libp2p/actions/runs/9029804098/job/24812857646?pr=1095.

It says an unhandled error was raised by testdaemon, but all tests passed. Do you know the proper way to fix that?

arnetheduck commented 1 month ago

It says an unhandled error was raised by testdaemon, but all tests passed. Do you know the proper way to fix that?

no, but it would be good if this was fixed.

diegomrsantos commented 5 days ago

It says an unhandled error was raised by testdaemon, but all tests passed. Do you know the proper way to fix that?

no, but it would be good if this was fixed.

It has been fixed here https://github.com/vacp2p/nim-libp2p/pull/1123