Use time.NewTimer() instead of time.After()

[afriza]

For efficiency, cancel time.Timer after no longer needed. See https://pkg.go.dev/time#After

Note on behavior change:
timeout duration <= 0 now means waiting forever instead of immediately times out..

[andreykaipov]

Very interesting, I didn't know time.After() leaks memory!

The underlying Timer is not recovered by the garbage collector until the timer fires.

Out of curiosity I wanted to see it for myself so I added profiling in #57 and sure enough:

❯ GOOBS_PROFILE=memprofile=mem.out OBS_PORT=4455 go test -v -run=profile client_test.go
❯ go tool pprof -top -sample_index=inuse_space mem.out
Type: inuse_space
Time: Sep 19, 2023 at 6:05pm (CDT)
Showing nodes accounting for 512.05kB, 100% of 512.05kB total
      flat  flat%   sum%        cum   cum%
  512.05kB   100%   100%   512.05kB   100%  time.NewTimer
         0     0%   100%   512.05kB   100%  command-line-arguments_test.Test_profile
         0     0%   100%   512.05kB   100%  github.com/andreykaipov/goobs/api.(*Client).SendRequest
         0     0%   100%   512.05kB   100%  github.com/andreykaipov/goobs/api/requests/scenes.(*Client).GetSceneList
         0     0%   100%   512.05kB   100%  testing.tRunner
         0     0%   100%   512.05kB   100%  time.After (inline)

Hope you don't mind I updated your PR to use defer timer.Stop() instead of potentially waiting forever. Found that in golang/go#27169 and it seems like it's hit others too. Thanks again!

[afriza]

@andreykaipov no problem on the updated PR. defer also works for efficiency.

But I am curious about:

1. As I understand, without the if in my newTimeoutTimer, the timeout duration <= 0 means immediately times out. So how is this better than waiting forever for c.IncomingResponses/authComplete? In my opinion, "no timeout" (duration <= 0) better fits waiting forever than immediately timing out.
2. I think the possibility of waiting forever mentioned in the golang issue is different from what I mean in (1) since the one in the golang issue is caused by deadlock while the one I mentioned in (1) is about interpretation of duration <= 0.

Thank you for the merge.

[andreykaipov]

My apologies -- I thought the motivation of the PR was only for the timer memory efficiency. Sorry for discarding your timeout changes without any comment.

I suppose I was concerned about backwards compatibility for anybody that might've been using a zero timeout already. However thinking about it now... that'd be impossible since immediately timing out wouldn't allow for the connection to establish in the first place.

I'm not against adding a way to specify an indefinite timeout but out of curiosity does using a really large timeout (e.g. 24 hours) not meet your needs?

[afriza]

@andreykaipov
The default timeout works for me and setting large timeout is not an issue for me if needed.
I agree that the need of unlimited timeout is almost as unreasonable as immediate timeout.
But in my opinion, unlimited makes more sense than immediate timeout.
I just wanted to clarify between the deadlock issue vs unlimited timeout.
Not really against immediate timeout though, but my preference is over unlimited in the case of a value <= 0 .
If you prefer to keep the existing behavior, that is fine as well.