-
Notifications
You must be signed in to change notification settings - Fork 112
fix: Nil dereference while using SetSendDontHaves #488
fix: Nil dereference while using SetSendDontHaves #488
Conversation
Thank you for submitting this PR!
Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment.
We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution. |
efcc3a1
to
2e5e6a8
Compare
@coryschwartz : we're hoping you can take a quick look at this as it may be reviewable without go-bitswap context. If you think you need more, then leave it for @aschmahmann . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM. Made a small suggestion on the documentation comments to make things a bit more explicit.
This option is used by the benchmark to simulate the old bitswap comportement. This follows the same refactoring idea as done in 47b99b1. It was crashing since it was trying to access the `sendDontHaves` property of `bs.engine` but `bs.engine` is initialized right after the options are applied, not before.
Co-authored-by: Adin Schmahmann <adin.schmahmann@gmail.com>
1ed9e96
to
3f031b4
Compare
Failing tests are unrelated. |
…e-if-using-SetSendDontHave-option fix: Nil dereference while using SetSendDontHaves This commit was moved from ipfs/go-bitswap@f2385cb
This option is used by the benchmark to simulate the old bitswap comportement (and fixes the benchmark).
This follows the same refactoring idea as done in 47b99b1.
It was crashing since it was trying to access the
sendDontHaves
property ofbs.engine
butbs.engine
is initialized right after the options are applied, not before.Unlike 47b99b1, I've choosed to not make
sendDontHaves
part of engine's constructor because this is such an obscure option, useless in +99% of realworld scenario that it's not needed to confuse the engine contructor.