Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use hinshun/cancel for cancellable readers without data being consumed #29

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hinshun
Copy link
Contributor

@hinshun hinshun commented Feb 10, 2022

Eliminate a lot of code with a proper cancellable reader.

Copy link
Contributor

@coryb coryb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, assuming the cancel readers works reliably this looks like a good improvement.

go.mod Outdated
@@ -4,5 +4,6 @@ go 1.13

require (
github.com/creack/pty v1.1.17
github.com/hinshun/cancel v0.0.0-20220210052008-740852a68445 // indirect
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

odd that it says indirect here, it is explicitly used in the proejct...

@hinshun
Copy link
Contributor Author

hinshun commented Feb 10, 2022

The Cancel() method returns bool to indicate whether it was successfully canceled. We could possibly plumb that somehow to the user, but there's no much they can do about it because its platform specific.

@hinshun
Copy link
Contributor Author

hinshun commented Feb 10, 2022

Looks like someone else already hoisted the package, so I'll let them maintain it:
https://github.com/muesli/cancelreader

Need to contrib some PRs there first though.

@hinshun
Copy link
Contributor Author

hinshun commented Feb 11, 2022

My patches were upstreamed to: https://github.com/muesli/cancelreader
And I've updated this PR accordingly.

We should probably test this with downstreams before considering merging this.

expect.go Outdated
Comment on lines 94 to 100
select {
case <-readCh:
case <-time.After(*readTimeout):
c.cancelReader.Cancel()
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use a timer we can explicitly stop here:

Suggested change
select {
case <-readCh:
case <-time.After(*readTimeout):
c.cancelReader.Cancel()
}
timer := time.NewTimer(*readTimeout)
select {
case <-readCh:
timer.Stop()
case <-timer.C:
c.cancelReader.Cancel()
}

Otherwise, the timers created by time.After will stay active after a read finishes. If reads are happening very fast, this can cause a lot of unnecessary timers to accumulate.

expect_test.go Outdated
func waitTestEnd(t *testing.T, done <-chan struct{}) {
select {
case <-done:
case <-time.After(100 * time.Millisecond):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be at least a few seconds to avoid flakiness on a heavily loaded system.

@aaronlehmann
Copy link

@hinshun: Might be nice to get this one wrapped up. It seemed like a worthwhile improvement.

…nsumed

Signed-off-by: Edgar Lee <edgarl@netflix.com>
@aaronlehmann
Copy link

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants