Feat: add idle_timeout to read_until #2263
Conversation
Signed-off-by: eduardodbr <[email protected]>
Signed-off-by: eduardodbr <[email protected]>
eduardodbr
changed the title
Signed-off-by: eduardodbr <[email protected]>
|
|
||
| if check == nil { | ||
| return nil, errors.New("a check query is required") | ||
| if check == nil && idleTimeout < 0 { |
There was a problem hiding this comment.
Wouldn't it be better to have <= 0 and initialise idleTimeout to 0 instead of -1? Not sure if 0 duration makes sense here.
There was a problem hiding this comment.
That also works. At first, my goal was to set it as negative to disable it, but I later realized that I didn't need to do that because we can use the empty string as a default value, so I guess I just forgot to change it to 0 since it still works like this. I'll update it, thanks
There was a problem hiding this comment.
LGTM, thanks @eduardodbr. One minor concern is whether there's a significant hit to having unbounded timers allocated from a busy input. Normally I'd rework this to use time.NewTimer which we can stop but I also remember reading about an optimisation in Go where that's no longer necessary, I can't find it right now though so maybe I dreamt it. I might rework it just incase.
|
Nice catch @Jeffail! I think that timer enhancement is still WIP: golang/go#61542 |
fixed #2249