codecov-commenter
commented
2 years ago Codecov Report
Merging #103 (1692c05) into master (5afc2de) will increase coverage by
0.25%. The diff coverage is0.00%.
@@ Coverage Diff @@
## master #103 +/- ##
=========================================
+ Coverage 7.56% 7.82% +0.25%
=========================================
Files 12 12
Lines 1758 1764 +6
=========================================
+ Hits 133 138 +5
- Misses 1621 1623 +2
+ Partials 4 3 -1 | Impacted Files | Coverage Δ | |
|---|---|---|
| filecoin/retriever.go | 0.00% <0.00%> (ø) |
|
| blocks/manager.go | 85.00% <0.00%> (+5.00%) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing dataPowered by Codecov. Last update 5afc2de...1692c05. Read the comment docs.
The lack of explicit timeout on query calls seems like an oversight to me. I'm not currently seeing endless connection attempts leaking in my local experiments but I imagine there's some kind of implicit timeout that's coming in to play in libp2p or some other network layer so it's probably cleaning up eventually. But eventually is not really what we want - queries should be quick, and we should be able to drop those that don't respond quickly. This PR reuses the per-miner timeout which defaults to 1m, but we could introduce a new one just for query phase and set it even shorter. The per-miner timeout timer is reset on bytes-received so it's probably reasonable that it's reused in this way I reckon.
The deadline should get reused as a libp2p socket deadline @ https://github.com/application-research/filclient/blob/cb126018ee48269269ceef8c5d39d3bf0806f5b3/filclient.go#L466-L470