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

Return errors from GetSession #1061

Merged
merged 2 commits into from
Jun 10, 2017
Merged

Return errors from GetSession #1061

merged 2 commits into from
Jun 10, 2017

Conversation

russjones
Copy link
Contributor

Purpose

GetSession() was hiding errors, this PR changes this behavior by returning errors other than trace.NotFound errors.

Implementation

  • Update GetSession to return non trace.NotFound errors.
  • Update GetSessions to propagate errors (this way we don't try and append a nil session to a slice).
  • In pollAndSync propagate the error if either we have an error or if the session is nil.

Related Issues

Fixes #1002

@russjones russjones requested a review from kontsevoy June 9, 2017 21:59
@russjones
Copy link
Contributor Author

@kontsevoy The only thing I am a little concerned with in this PR is that this might be expected behavior and this PR will break things unexpectedly. In a couple of places in the code it seems we look for session being nil rather than an error.

Copy link
Contributor

@kontsevoy kontsevoy left a comment

Choose a reason for hiding this comment

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

Do we log the error from GetSessions() though? I do not see it anywhere. Would like to avoid silent errors.

@kontsevoy
Copy link
Contributor

@russjones well... even if we return an error now, the returned session is still nil so the existing code should continue operating.

@russjones russjones force-pushed the rjones/fix-err-check branch from 0ff29e2 to 97b5b8f Compare June 9, 2017 23:57
@russjones
Copy link
Contributor Author

@kontsevoy I think we're logging it on some code paths. When I force an error in GetSession() I see logs in web/stream.go and srv/sess.go but I don't think that covers all code paths so I added another log line in GetSessions() like you suggested.

@kontsevoy kontsevoy merged commit 752ee0f into master Jun 10, 2017
@kontsevoy kontsevoy deleted the rjones/fix-err-check branch June 10, 2017 04:33
hatched pushed a commit to hatched/teleport-merge that referenced this pull request Nov 30, 2022
If '0' is provided as the default and the initial call to create a gateway
fails, the user will be presented with an input that says 0. 0 is an invalid
value though that this input will not accept.

An empty string, on the other hand, is a perfectly valid value (the input
doesn't have the `required` attribute). In that scenario, the backend will
default the value to '0' and pick a random port.
hatched pushed a commit that referenced this pull request Dec 20, 2022
If '0' is provided as the default and the initial call to create a gateway
fails, the user will be presented with an input that says 0. 0 is an invalid
value though that this input will not accept.

An empty string, on the other hand, is a perfectly valid value (the input
doesn't have the `required` attribute). In that scenario, the backend will
default the value to '0' and pick a random port.
hatched pushed a commit that referenced this pull request Feb 1, 2023
If '0' is provided as the default and the initial call to create a gateway
fails, the user will be presented with an input that says 0. 0 is an invalid
value though that this input will not accept.

An empty string, on the other hand, is a perfectly valid value (the input
doesn't have the `required` attribute). In that scenario, the backend will
default the value to '0' and pick a random port.
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.

2 participants