Regression in 0.0.2 when Unlimited bandwidth is used

[jkowalski]

I've noticed some of my tests started failing after upgrading to 0.0.2 and iothrottler was on stack traces of many failures:

https://github.com/kopia/kopia/runs/2156615830?check_suite_focus=true

and those tests all use Unlimited bandwidth.

I think there's a bug due to integer overflow. The easiest repro I could find is this to add some real-time sleep:

func TestUnlimitedBandwidthIsFast(t *testing.T) {
	pool := iothrottler.NewIOThrottlerPool(iothrottler.Unlimited)
	readEnd, writeEnd := io.Pipe()
	throttledReadEnd, err := pool.AddReader(readEnd)
	throttledWriteEnd, err := pool.AddWriter(writeEnd)
	assertNoError(t, err)
	data := make([]byte, 10000)

	assertTransmitTime(data, throttledReadEnd, throttledWriteEnd, 0, t)
	assertTransmitTime(data, throttledReadEnd, throttledWriteEnd, 0, t)
	// sleep for some time
	time.Sleep(1 * time.Second)
	assertTransmitTime(data, throttledReadEnd, throttledWriteEnd, 0, t)
}

With this I got the failure on third assertTransmitTime()

iothrottler_test.go:586: Expecting read to take 0 seconds but it took 1 instead

I did not look too deeply, but I think the overflow is in useBandwidth():

int((secondsElapsed * int64(pool.bandwidth)) - pool.bandwidthUsed)

if secondsElapsed > 0 it will overflow int64 range.

FWIW this seems to fix it:

	var availableBandwidth int

	if pool.bandwidth == Unlimited {
		availableBandwidth = Unlimited
	} else {
		availableBandwidth = int((secondsElapsed * int64(pool.bandwidth)) - pool.bandwidthUsed)
	}

but it's probably not the right fix (won't work for Unlimited - 1 for example) and there are many more corner cases.

I'm going to revert Kopia to 0.0.1 for now, will be happy to test any fixes here.

BTW thanks for this library, it is really easy to use!

[efarrer]

@jkowalski Thank you for the detailed report and sorry I introduced a regression. I'm investigating fixes and will hopefully have a fix soon.

[jkowalski]

Is there any update on this? I like to keep all my dependencies on latest versons but I can't upgrade iothrottler until this is fixed.

[efarrer]

@jkowalski I apologize for the delay in addressing this. I'm going to chalk this one up to COVID fog. I discovered that my refactor was misguided, it made the code more efficient, but also it didn't work ;). I've reverted the refactor and added the above test case as a regression test so I won't break it (that way) in the future. I've pushed out v0.0.3 which is the same as v0.0.1 but with the above test code. There are some improvements that I can make, but this at least allows you to upgrade your dependencies to the latest.