nats.go icon indicating copy to clipboard operation
nats.go copied to clipboard

Remove globalTimerPool global variable

Open jipperinbham opened this issue 2 years ago • 2 comments

We occasionally see weird behavior when using Request with timeouts where it appeared as though the timer had already fired even though it did not appear to be the case. Looking through the Request functions, it seemed pragmatic to rely on the standard library's context instead. The other use of globalTimerPool in NextMsg and FlushTimeout appeared to also be able to use context.WithTimeout which made it no longer necessary to have the globalTimerPool variable.

Looking through some open issues, it appeared as though https://github.com/nats-io/nats.go/issues/880 could be addressed with this change.

jipperinbham avatar Apr 07 '22 17:04 jipperinbham

Coverage Status

Coverage decreased (-0.09%) to 85.159% when pulling 4533c937c395c23643dd1623451cabb414f1a7ca on jipperinbham:jp/no-global-pool into c4cfcdf430719cd3e02ded4b5412c0925fcbfb64 on nats-io:main.

coveralls avatar Apr 07 '22 18:04 coveralls

Thanks for the contribution. I think this might be preventing the issue because context is not using a pool for those timers and avoiding what looks like were timer regressions in go 1.16, 1.7 that have been fixed since then? (https://github.com/golang/go/issues/47859) Internally context.WithTimeout uses AfterFunc so there would be a goroutine per request, not sure about changing that into being the default yet... Also have you tried with switching to RequestWithContext instead?

wallyqs avatar Apr 07 '22 18:04 wallyqs