-
Notifications
You must be signed in to change notification settings - Fork 55
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
DXCDT-550: Propagate errors through websocket connection #879
DXCDT-550: Propagate errors through websocket connection #879
Conversation
h.shutdown() | ||
return | ||
} | ||
defer func() { | ||
_ = connection.Close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We weren't properly closing the connection before. This is really important to do.
if websocket.IsCloseError(err, websocket.CloseNormalClosure, websocket.CloseGoingAway) || | ||
websocket.IsUnexpectedCloseError(err, websocket.CloseAbnormalClosure) { | ||
// The connection was closed. | ||
break | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic is needed in order to prevent a panic where we keep trying to read from a closed connection due to the for loop.
|
||
errorMsg := webSocketMessage{ | ||
Type: errorMessageType, | ||
Payload: []byte(fmt.Sprintf(`{"error":%q}`, err.Error())), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be using a struct to represent the error, and then JSON-encode it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, we should. I plan to do that in a follow up PR and refactor the entire webSocketMessage so we can add custom JSON marshalling and unmarshalling and reuse the payload accordingly. This will prevent us from needing to unmarshal twice the message when we received it through the websocket as we're doing now.
The solution would be similar to what we do within the Go SDK for Connection Options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
errorMsg := webSocketMessage{ | ||
Type: errorMessageType, | ||
Payload: []byte(fmt.Sprintf(`{"error":%q}`, err.Error())), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
64c08a4
into
feature/auth0-universal-login-customize
🔧 Changes
This PR adds the ability to propagate errors through the WebSocket connection to the Web App used through the
auth0 universal-login
customize command. The payload of this message is as follows:📚 References
🔬 Testing
📝 Checklist