Exchange Head single-flight protection #229
Conversation
IliaBulavintsev
requested review from
tzdybal,
renaynay,
Wondertan,
vgonkivs,
gupadhyaya,
walldiss and
cristaloleg
as code owners
|
Hey @IliaBulavintsev, thanks for the contribution. Note that the issue this PR targets is meant to cover all the methods and not only |
Hey @Wondertan ! Thank you for your review. The implementation was meant to only cover |
There was a problem hiding this comment.
This is a great start :) just a few things
- can you please remove the
sync_getterfrom the syncer component as now the single flight protection has moved to the exchange instead? - re tests: they are comprehensive and have good coverage, but I think they may be a bit overkill. Testing 2 to max 3 simultaneous requests would be sufficient. Also the
requestFromTrustedparam in the tests would be more useful in terms of coverage if it was false for some cases. - nit: the tests could benefit from a bit of spacing between statements for readability :)
- test case with untrusted peer
|
Hey @renaynay!
Lmk if you have other feedback |
renaynay
changed the title
p2p/exchange_test.go
Outdated
| { | ||
| requestFromTrusted: true, | ||
| lastHeader: trustedStore.Headers[trustedStore.HeadHeight-1], | ||
| expectedHeight: trustedStore.HeadHeight, | ||
| expectedHash: trustedStore.Headers[trustedStore.HeadHeight].Hash(), | ||
| }, | ||
| { | ||
| requestFromTrusted: true, | ||
| lastHeader: trustedStore.Headers[trustedStore.HeadHeight-1], | ||
| expectedHeight: trustedStore.HeadHeight, | ||
| expectedHash: trustedStore.Headers[trustedStore.HeadHeight].Hash(), | ||
| }, |
There was a problem hiding this comment.
what's the difference between these two test cases?
There was a problem hiding this comment.
The duplicate cases serve to double-check that singleflight consistently returns the same response for identical requests from a trusted source. However, if you think a single case is sufficient for validation, we can remove the duplicates to streamline the test
There was a problem hiding this comment.
That seems like a good test generally-speaking but I don't think it is relevant to the single-flight protection feature and should rather be implemented in a different way. I appreciate the thought here, however.
There was a problem hiding this comment.
Got it, I’ll remove the test as it’s not directly relevant to single-flight. Thanks for the feedback!
#154
Overview
This PR adds singleflight protection to prevent duplicate header requests in the Exchange component. The focus is on minimizing redundant requests, especially around the Head method, which is most likely to experience simultaneous calls.