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

Add ReadTimeoutHandler and ReadTimeout to connection #10

Merged
merged 3 commits into from
Jun 14, 2022
Merged

Conversation

markalex
Copy link
Contributor

  • Adds ReadTimeout property and ReadTimeoutHandler callback function to facilitate timeout handling when acting in Server mode.

Copy link

@atonks2 atonks2 left a comment

Choose a reason for hiding this comment

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

Nice. Looks like this ended up being fairly straightforward.

@@ -18,10 +18,18 @@ type Options struct {
// message to the server
IdleTime time.Duration

// ReadTimeout is the maximum time between read events before the
// ReadTimeoutHandler is called
ReadTimeout time.Duration
Copy link
Member

@adamdecaf adamdecaf Jun 14, 2022

Choose a reason for hiding this comment

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

I think we need a default value for this, otherwise <-time.After(0*time.Second) would fire endlessly.

See: https://go.dev/play/p/gOlaqNI-UqT

Note: The Go playground hardcodes time to a static value.

Copy link
Member

Choose a reason for hiding this comment

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

This does fire endlessly when ran locally:

when: 2022-06-14 16:38:50.354385 -0500 CDT m=+1.800189243
when: 2022-06-14 16:38:50.354388 -0500 CDT m=+1.800192336
when: 2022-06-14 16:38:50.354392 -0500 CDT m=+1.800196537
when: 2022-06-14 16:38:50.354394 -0500 CDT m=+1.800199192
when: 2022-06-14 16:38:50.354398 -0500 CDT m=+1.800203148
when: 2022-06-14 16:38:50.354401 -0500 CDT m=+1.800205735
when: 2022-06-14 16:38:50.354405 -0500 CDT m=+1.800209760
when: 2022-06-14 16:38:50.354425 -0500 CDT m=+1.800229379
when: 2022-06-14 16:38:50.354427 -0500 CDT m=+1.800231794
when: 2022-06-14 16:38:50.354429 -0500 CDT m=+1.800234150
when: 2022-06-14 16:38:50.354455 -0500 CDT m=+1.800259537
when: 2022-06-14 16:38:50.359297 -0500 CDT m=+1.805102074

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good call

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I set it to 60 seconds, which is longer than the 45 seconds we'll use in our implementation.

connection.go Show resolved Hide resolved
@adamdecaf
Copy link
Member

FYI the gitleaks error is legit, but we're unable to exclude files right now. There's an open issue to support this.

@mhargrove
Copy link

FYI the gitleaks error is legit, but we're unable to exclude files right now. There's an open issue to support this.

as a workaround in the meantime would we want to maybe move the private key(s) to vault or bake the file(s) into the image or something? (though for testdata that feels a bit excessive..)

@adamdecaf
Copy link
Member

Vault isn't part of this project. We could just disable gitleaks for now. The gitleaks project needs some help to support our usecase.

@adamdecaf
Copy link
Member

Flake on the test run? Or a legit failure?

@markalex markalex merged commit 5e802f6 into master Jun 14, 2022
@markalex markalex deleted the read-timeout branch June 14, 2022 23:58
@alovak
Copy link
Contributor

alovak commented Jun 15, 2022

Thanks for the PR! Looks great!

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

5 participants