Skip to content
This repository has been archived by the owner on Feb 1, 2023. It is now read-only.

fix: Nil dereference while using SetSendDontHaves #488

Conversation

Jorropo
Copy link
Contributor

@Jorropo Jorropo commented May 30, 2021

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 of bs.engine but bs.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.

@welcome
Copy link

welcome bot commented May 30, 2021

Thank you for submitting this PR!
A maintainer will be here shortly to review it.
We are super grateful, but we are also overloaded! Help us by making sure that:

  • The context for this PR is clear, with relevant discussion, decisions
    and stakeholders linked/mentioned.

  • Your contribution itself is clear (code comments, self-review for the
    rest) and in its best form. Follow the code contribution
    guidelines

    if they apply.

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.
Next steps:

  • A maintainer will triage and assign priority to this PR, commenting on
    any missing things and potentially assigning a reviewer for high
    priority items.

  • The PR gets reviews, discussed and approvals as needed.

  • The PR is merged by maintainers when it has been approved and comments addressed.

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.
We are very grateful for your contribution!

@Jorropo Jorropo force-pushed the fix/nildereferance-if-using-SetSendDontHave-option branch from efcc3a1 to 2e5e6a8 Compare May 30, 2021 09:22
@Jorropo
Copy link
Contributor Author

Jorropo commented May 30, 2021

/cc @dirkmc (no idea if you are interessed but you wrote 47b99b1 :))

@aschmahmann aschmahmann added effort/hours Estimated to take one or several hours exp/beginner Can be confidently tackled by newcomers P2 Medium: Good to have, but can wait until someone steps up labels Jun 7, 2021
@aschmahmann aschmahmann self-assigned this Jun 7, 2021
@BigLep
Copy link

BigLep commented Jun 7, 2021

@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 .

Copy link

@coryschwartz coryschwartz left a 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.

Copy link
Contributor

@aschmahmann aschmahmann left a 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.

bitswap.go Outdated Show resolved Hide resolved
Jorropo and others added 2 commits June 24, 2021 10:46
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>
@Stebalien Stebalien force-pushed the fix/nildereferance-if-using-SetSendDontHave-option branch from 1ed9e96 to 3f031b4 Compare June 24, 2021 17:47
@Stebalien Stebalien merged commit f2385cb into ipfs:master Jun 24, 2021
@Stebalien
Copy link
Member

Failing tests are unrelated.

@Jorropo Jorropo deleted the fix/nildereferance-if-using-SetSendDontHave-option branch June 24, 2021 18:36
@aschmahmann aschmahmann mentioned this pull request Aug 23, 2021
62 tasks
Jorropo pushed a commit to Jorropo/go-libipfs that referenced this pull request Jan 26, 2023
…e-if-using-SetSendDontHave-option

fix: Nil dereference while using SetSendDontHaves

This commit was moved from ipfs/go-bitswap@f2385cb
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
effort/hours Estimated to take one or several hours exp/beginner Can be confidently tackled by newcomers P2 Medium: Good to have, but can wait until someone steps up
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants