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

r := io.LimitReader(rread, int64(77*rand.Intn(123))) can create a 0 sized reader causing unittest(s) to panic and fail #50

Closed
nitishm opened this issue Dec 26, 2018 · 2 comments

Comments

@nitishm
Copy link

nitishm commented Dec 26, 2018

go-mfs/mfs_test.go

Lines 562 to 567 in e5a375d

rread := rand.New(rand.NewSource(time.Now().UnixNano()))
r := io.LimitReader(rread, int64(77*rand.Intn(123)))
_, err = io.Copy(wfd, r)
if err != nil {
return err
}

In Line 563 the random generator can generate all values [0,n) https://golang.org/pkg/math/rand/#Rand.Intn, which can in-turn create a io.LimitReader with 0 size. Since this is copied into the newly created fileDescriptor (writer) at https://github.com/ipfs/go-mfs/blob/e5a375ded4ae3dfbf6fca4eb1c5e606f19fdc676/mfs_test.go#L564 it can propagate the failure up to test cases that use helpers that panic at 0 size, for example, https://github.com/ipfs/go-mfs/blob/e5a375ded4ae3dfbf6fca4eb1c5e606f19fdc676/mfs_test.go#L642.

With my latest changes in PR #49, I see this happening more often, (dont have an explanation for why this is surfacing now, but still seems to be random at best), causing TestMfsStress() to fail.

Proposal : Check the randomly generated size before trying to create the Reader

rread := rand.New(rand.NewSource(time.Now().UnixNano()))
	limitReaderSize := int64(77*rand.Intn(123))
	for ;limitReaderSize == 0; {
		limitReaderSize = int64(77*rand.Intn(123))
	}
	r := io.LimitReader(rread, limitReaderSize)
@schomatis
Copy link
Contributor

Yes, that isn't nice.

Just add a +1 to the multiplication (and document the line explaining that we don't want to create empty files), push that commit to your ongoing PR and keep working on it to see if that helps.

If that was indeed the problem you're one unlucky guy 😄

@nitishm
Copy link
Author

nitishm commented Dec 26, 2018

Hah, fairly certain that fixes the problem. I tried running the test multiple time with and without the +1 and it passes with it.

I will push the change to #49

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants