-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[No QA] Improve logging for Pusher error
event. Avoid unnecessary reauthentication. Update Pusher.
#8635
Conversation
error
event. Avoid unnecessary reauthentication.
error
event. Avoid unnecessary reauthentication.error
event. Avoid unnecessary reauthentication.
error
event. Avoid unnecessary reauthentication.error
event. Avoid unnecessary reauthentication. Update Pusher.
|
||
reject(status); | ||
channel.bind('pusher:subscription_error', (data) => { | ||
const {type, error, status} = data; |
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.
The signature of this error event has changed. Previously we were throwing an error that could be caught by any caller of Pusher.subscribe()
. This can be an AuthError
and then would get thrown with a reject(undefined)
. So, this change gives us a bit more information about what went wrong. I think for the most part this would be an AuthError
(what would happen when the callback()
in the custom authorizer returns an error).
Log.hmmm('[PusherConnectionManager] WebSocketError', {error}); | ||
} else { | ||
Log.alert(`${CONST.ERROR.ENSURE_BUGBOT} [PusherConnectionManager] Unknown error event`, {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.
I know this stuff is a little redundant, but I'm enumerating these and logging an alert so we can clearly document all the errors that can possibly happen.
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.
Great addition
Log.info('[PusherConnectionManager] Unhandled error: ', false, {channelName}); | ||
callback(error, {auth: ''}); | ||
Log.hmmm('[PusherAuthorizer] Unhandled error: ', {channelName, error}); | ||
callback(new Error('Push_Authenticate request failed'), {auth: ''}); |
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.
I'm not re-throwing this error because the error that can show up in a catch doesn't really offer much information about why the request failed anyway. It could have:
- run out of retries
- server returned 403 Forbidden header (which we can't tell from the JS)
- User doesn't have access to a report
- Channel name isn't valid
- etc...
@@ -318,7 +318,14 @@ function subscribeToUserEvents() { | |||
}, false, | |||
() => { | |||
NetworkConnection.triggerReconnectionCallbacks('pusher re-subscribed to private user channel'); | |||
}); | |||
}) | |||
.catch((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.
This needed a catch as it was causing an unhandled promise rejection.
@@ -16,16 +17,20 @@ function init() { | |||
})); | |||
|
|||
/** | |||
* Events that happen on the pusher socket are used to determine if the app is online or offline. | |||
* The offline setting is stored in Onyx so the rest of the app has access to 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.
Removing this as it is not accurate. The events are not used to determine online/offline status.
While testing this locally I can see that after disconnecting once I reconnect it doesn't throw any more error and the last command that it executes on server was |
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.
Ohh thanks, it works as expected now. For code, I think it will be better if someone else with experience writing code related to pusher can look into it because this is definitely a good upgrade over what we already had. As part of my review, it looked good to me.
Good call. I will ask for some help! |
src/libs/PusherConnectionManager.js
Outdated
switch (eventName) { | ||
case 'error': | ||
Log.info('[PusherConnectionManager] error event', false, {error: data}); | ||
Session.reauthenticatePusher(); | ||
if (error && error.type === 'PusherError' && error.data.code === 1006) { |
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.
NAB question, we are guaranteed to have the data
field be present with code
defined?
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.
(given an error
event name)
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.
Ah good catch. I think it's probably unsafe to assume that.
src/libs/PusherConnectionManager.js
Outdated
Session.reauthenticatePusher(); | ||
if (error && error.type === 'PusherError' && error.data.code === 1006) { | ||
Log.hmmm('[PusherConnectionManager] Channels Error 1006', {error}); | ||
} else if (error && error.type === 'PusherError' && error.data.code === 4201) { |
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.
Is there a place where these seemingly-arbituary integer error codes are referenced?
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.
(aka could we add this as a comment to reference?)
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.
Yeah, I think I can do that.
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.
Makes sense, just a few open questions
Updated 🙃 |
Updated |
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.
Tested with your steps (borked the authToken, killed wifi ~ 20 seconds, reconnected) and saw successful pusher connection after 👍
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by @marcaaron in version: 1.1.57-0 🚀
|
🚀 Deployed to production by @chiragsalian in version: 1.1.57-17 🚀
|
Details
error
eventerror
events are not related to authentication and trigger when there are issues with websocketsFixed Issues
$ #8683
Tests
PR Reviewer Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
).src/languages/*
filesSTYLE.md
) were followed/** comment above it */
displayName
propertythis
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)QA Steps
No QA
Screenshots
Web
Mobile Web
Desktop
iOS
Android