Add WithContext and WithTimeout #92

[gtourkas]

Overall I've followed the in-issue guide. Still need to look into a suspicious defer inside retryRequest but this will suffice for review.

[gavv]

Thanks for the PR! Here are a few comments.

[gtourkas]

@gavv Pushed all the changes you requested.

[gavv]

Thanks for update! A few final minor comments.

[gavv]

Still need to look into a suspicious defer inside retryRequest

Looks legit

[gavv]

It seems there is a possible deadlock in the new tests.

I've run this:

go test -race -count=100 -v |& pp

(pp is https://github.com/maruel/panicparse)

And eventually TestPerRequestWithTimeoutAndWithRetries stucked and then terminated by timeout: https://gist.github.com/gavv/e9aa492ed0ca50e8073251099e9f9f7c

[gavv]

(Also it seems that goroutines are leaking in TestE2EWebsocketTimeouts, but I think this is unrelated; those goroutines are excluded from trace)

[gtourkas]

It seems there is a possible deadlock in the new tests.

I've run this:

go test -race -count=100 -v |& pp

(pp is https://github.com/maruel/panicparse)

And eventually TestPerRequestWithTimeoutAndWithRetries stucked and then terminated by timeout: https://gist.github.com/gavv/e9aa492ed0ca50e8073251099e9f9f7c

I'll look into this shortly.

[gtourkas]

TestPerRequestWithTimeoutAndWithRetries

It seems there is a possible deadlock in the new tests.
I've run this:

go test -race -count=100 -v |& pp

(pp is https://github.com/maruel/panicparse)
And eventually TestPerRequestWithTimeoutAndWithRetries stucked and then terminated by timeout: https://gist.github.com/gavv/e9aa492ed0ca50e8073251099e9f9f7c

I'll look into this shortly.

@gavv can you reproduce with

go test -race -count=100 -v -run TestPerRequestWithTimeoutAndWithRetries

?

[gavv]

Hm, yes, it's reproducing, but only without -run TestPerRequestWithTimeoutAndWithRetries. This time TestPerRequestWithTimeout was the failed test.

I guess that with -run conditions for the race are not met.

Another opportunity is that the problem is indirectly caused by other tests, e.g. by gorotuine leak mentioned above - I didn't try to debug it yet.

My env:

$ git status
On branch gtourkas/master
nothing to commit, working tree clean
$ git rev-parse HEAD
b06385b7aed0d723773e5aca4339881ed6c712fe
$ go version
go version go1.16.2 linux/amd64
$ uname -a
Linux tripbook 4.19.0-14-amd64 #1 SMP Debian 4.19.171-2 (2021-01-30) x86_64 GNU/Linux

[gtourkas]

@gavv I can reproduce as well. The problem is with the WithTimeout tests. There is no problem when omitted. I'll try to debug.

[gtourkas]

@gavv It seems that the problem is the default timeout for go test, which is 10m(inutes). A single test run lasts in my environment 6.317s(econds). For -count=100, this will be at least 631.7s>10m. Simply increasing the timeout to 11m will result in passing the test run:

go test -count=100 -timeout=11m

In any case, I've set the various WithTimeouts to 500ms so that the tests complete under the default 10m for count=100.

[gavv]

Never mind, I expected the timeout is per-test... Thanks for looking into it and for the PR!