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

Detect and handle vici client connection errors #26

Merged
merged 1 commit into from
Mar 8, 2021

Conversation

Crevil
Copy link
Contributor

@Crevil Crevil commented Mar 8, 2021

Currently ClientConn starts a go routine on construction that runs the data read
loop. Errors from this read loop is reported on ClientConn.lastError but as any
request on the client uses ClientConn.readResponse to receive data and that
function will timeout if the underlying connection has issues the
ClientConn.lastError is overwritten with a timeout error. This results in an
invisible and non-recoverable error state of the client.

This change moves the read loop into an exported Listen method that clients are
required to call to start the clients read loop. This bringes clients in control
of the error handling and avoids the subtle bug on error reporting through
ClientConn.lastError.

The main function is updated accordingly to start the client listeners in Go
routines and handle shutdown appropriately.

For better error reporting a String method is added to segmentType so logs will
write the human readable name of the segment instead of the integer based byte
value.

Currently ClientConn starts a go routine on construction that runs the data read
loop. Errors from this read loop is reported on ClientConn.lastError but as any
request on the client uses ClientConn.readResponse to receive data and that
function will timeout if the underlying connection has issues the
ClientConn.lastError is overwritten with a timeout error. This results in an
invisible and non-recoverable error state of the client.

This change moves the read loop into an exported Listen method that clients are
required to call to start the clients read loop. This bringes clients in control
of the error handling and avoids the subtle bug on error reporting through
ClientConn.lastError.

The main function is updated accordingly to start the client listeners in Go
routines and handle shutdown appropriately.

For better error reporting a String method is added to segmentType so logs will
write the human readable name of the segment instead of the integer based byte
value.
@Crevil Crevil requested a review from a team March 8, 2021 12:33
@Crevil Crevil merged commit c2adff6 into master Mar 8, 2021
@Crevil Crevil deleted the fix/vici-client-connection-reporting branch March 8, 2021 12:38
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