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

Move fetcher feeding to separate goroutine to avoid deadlocking #3550

Closed
wants to merge 1 commit into from

Conversation

Voker57
Copy link
Contributor

@Voker57 Voker57 commented Dec 29, 2016

EnumerateChildrenAsync is sending slices of keys to fetchNodes, but receives only one node per iteration, which, when coupled with recent addition of concurrency limit, can lead to deadlock when toprocess is full but nodes is empty. This patch defers feeding toprocess to separate (unbounded) goroutines to allow concurrent IO between the two functions.

Fixes #3505

@Voker57 Voker57 force-pushed the enumerate-deadlock-fix branch 4 times, most recently from 6ce0746 to a84fe76 Compare January 2, 2017 06:50
@Kubuxu
Copy link
Member

Kubuxu commented Jan 4, 2017

Could you make it panic safe and note use recover?

@Voker57
Copy link
Contributor Author

Voker57 commented Jan 4, 2017

I'll try.

@Voker57 Voker57 force-pushed the enumerate-deadlock-fix branch from a84fe76 to 9e12323 Compare January 6, 2017 04:54
@Voker57
Copy link
Contributor Author

Voker57 commented Jan 6, 2017

@Kubuxu done. CI seems to be broken but my test passed

@whyrusleeping
Copy link
Member

@Kubuxu lets make sure to review this one thoroughly, this part of the codebase always feels a tad fragile to me

@whyrusleeping whyrusleeping added this to the ipfs 0.4.5 milestone Jan 6, 2017
@Kubuxu
Copy link
Member

Kubuxu commented Jan 6, 2017

It does and is, as the semaphore change shows.

@whyrusleeping
Copy link
Member

@Voker57 Thanks for this fix, but we've decided that this part of the codebase needed to just get rewritten. I'll close this in favor of #3571 (and please do make sure that PR looks good to you)

@Voker57
Copy link
Contributor Author

Voker57 commented Jan 14, 2017

closed in favor of #3598

@Voker57 Voker57 closed this Jan 14, 2017
@Voker57 Voker57 deleted the enumerate-deadlock-fix branch July 15, 2017 10:00
@Voker57 Voker57 restored the enumerate-deadlock-fix branch November 1, 2017 14:06
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