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 t.Fatal in a goroutine #2062

Merged
merged 4 commits into from
Dec 15, 2015
Merged

fix t.Fatal in a goroutine #2062

merged 4 commits into from
Dec 15, 2015

Conversation

chriscool
Copy link
Contributor

This should fix at least part of issue #2043.

License: MIT
Signed-off-by: Christian Couder chriscool@tuxfamily.org

@jbenet jbenet added the status/in-progress In progress label Dec 12, 2015
@chriscool chriscool force-pushed the fix-t-fatal-goroutine branch 3 times, most recently from 59653ef to 1bc2ebd Compare December 12, 2015 20:53
@chriscool chriscool changed the title bitswap_test: fix t.Fatal in a goroutine fix t.Fatal in a goroutine Dec 12, 2015
@chriscool
Copy link
Contributor Author

Test failures are in Sharness tests and are not related.

@@ -142,6 +142,7 @@ func sizeOfIthFile(i int64) int64 {
}

func runFileAddingWorker(n *core.IpfsNode) error {
errs := make(chan error, math.MaxInt32)
Copy link
Member

Choose a reason for hiding this comment

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

what... what is going on in this test? MaxInt32 ?

your changes look good to me, but we should probably reevaluate what this test is doing...

Copy link
Member

Choose a reason for hiding this comment

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

i believe this thing used to just sit, adding a new file every second. it could just as well loop forever.

Copy link
Member

Choose a reason for hiding this comment

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

it's not really used.

@whyrusleeping
Copy link
Member

Looks good! this may help flush out some issues, especially the ones in bitswap. Those might be the cause of our occasional bitswap test hang.

Only thing I would do is to not buffer the error channels, the pattern youre using should work fine with no buffering.

@chriscool
Copy link
Contributor Author

@whyrusleeping ok thanks for your review.

I will wait a bit before for @jbenet or someone else's opinion before removing the error channel buffers though.

@@ -168,19 +168,31 @@ func PerformDistributionTest(t *testing.T, numInstances, numBlocks int) {
t.Log("Distribute!")

wg := sync.WaitGroup{}
errs := make(chan error, 10)
Copy link
Member

Choose a reason for hiding this comment

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

here this could be make(chan error, len(instances))

Copy link
Member

Choose a reason for hiding this comment

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

(or no buffering)

@jbenet
Copy link
Member

jbenet commented Dec 13, 2015

Only thing I would do is to not buffer the error channels, the pattern youre using should work fine with no buffering.

yeah that sounds good to me, no buffering is fine

@jbenet
Copy link
Member

jbenet commented Dec 13, 2015

this LGTM, other than no buffers

License: MIT
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
License: MIT
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
License: MIT
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
License: MIT
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
@chriscool chriscool force-pushed the fix-t-fatal-goroutine branch from 1bc2ebd to 57c4188 Compare December 14, 2015 21:33
@chriscool
Copy link
Contributor Author

Ok, I removed all the buffering.

jbenet added a commit that referenced this pull request Dec 15, 2015
@jbenet jbenet merged commit df65bae into master Dec 15, 2015
@jbenet jbenet removed the status/in-progress In progress label Dec 15, 2015
@jbenet jbenet deleted the fix-t-fatal-goroutine branch December 15, 2015 19:13
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