Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(bitswap/client): dont set nil for DontHaveTimeoutConfig #872

Merged
merged 3 commits into from
Mar 7, 2025

Conversation

Wondertan
Copy link
Member

I thought I was going nuts... That's not even funny.

Copy link
Contributor

@guillaumemichel guillaumemichel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Wondertan!

Copy link
Contributor

@guillaumemichel guillaumemichel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, since bs.dontHaveTimeoutConfig is only used to define peerQueueFactory, setting it to nil should have no effect on the closure previously defined. This looks like a memory optimization.

Any opinions @gammazero?

@Wondertan
Copy link
Member Author

@guillaumemichel, closure takes value from Client struct tho, so this nil sets the config entirely to zero even if its set.

The DontHaveTimeout component then makes a default value silently if nil is given, so in the end DontHaveTimeout component works but with default config even when user passes a modified config.

This is the reason why I was going "nuts". I spent too much time debugging the config and my understanding of Bitswap code there, while this was an issue...

@gammazero
Copy link
Contributor

Actually, since bs.dontHaveTimeoutConfig is only used to define peerQueueFactory, setting it to nil should have no effect on the closure previously defined. This looks like a memory optimization.

Any opinions @gammazero?

The values inside in the function are not evaluated until the function is run, so when peerQueueFactory is called, bs.perPeerSendDelay will appear to be nil. This is nothing more than an attempt to dispose of memory that is not needed, but it affects what it inside the closure.

@Wondertan
Copy link
Member Author

should we make it skipchangelog?

@gammazero
Copy link
Contributor

gammazero commented Mar 7, 2025

should we make it skipchangelog?

I think it would be good make a note that we no longer ignore DontHaveTimeoutConfig. If there is a change in functionality or in the way something is used, then it should be noted in change log. I think this qualifies.

@gammazero gammazero merged commit f1d5312 into ipfs:main Mar 7, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants