Skip to content

Fix various post sign out requests + bugs #1075

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.

Sign up for GitHub

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

Merged
marcaaron merged 9 commits into from
Dec 30, 2020
Merged

Fix various post sign out requests + bugs #1075

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6b5213e
Fix various post sign out requests + bugs
marcaaron Dec 23, 2020
218d0c2
clear timers
marcaaron Dec 23, 2020
a852fef
fix comment
marcaaron Dec 23, 2020
2b2bb4a
remove clear queue method
marcaaron Dec 23, 2020
75fbb0f
fix conflicts
marcaaron Dec 29, 2020
ce167cb
Add clearRequestQueue() and clear the queue when there is no authToken
marcaaron Dec 29, 2020
82e21d0
Fix up comment
marcaaron Dec 29, 2020
9f08e55
fix comment again
marcaaron Dec 29, 2020
e97a947
last one
marcaaron Dec 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions src/libs/API.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,14 @@ function addAuthTokenToParameters(command, parameters) {
const finalParameters = {...parameters};

if (isAuthTokenRequired(command) && !parameters.authToken) {
// If we end up here with no authToken it means we are trying to make
// an API request before we are signed in. In this case, we should just
// cancel this and all other requests and set isAuthenticating to false.
// If we end up here with no authToken it means we are trying to make an API request before we are signed in.
// In this case, we should cancel the current request by pausing the queue and clearing the remaining requests.
if (!authToken) {
console.debug('A request was made without an authToken', {command, parameters});
Network.unpauseRequestQueue();
redirectToSignIn();

console.debug('A request was made without an authToken', {command, parameters});
Network.pauseRequestQueue();
Network.clearRequestQueue();
return;
}

Expand Down
14 changes: 14 additions & 0 deletions src/libs/Network.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,12 @@ function processNetworkRequestQueue() {
? enhanceParameters(queuedRequest.command, queuedRequest.data)
: queuedRequest.data;

// Check to see if the queue has paused again. It's possible that a call to enhanceParameters()
// has paused the queue and if this is the case we must return.
if (isQueuePaused) {
return;
}
Copy link
Contributor Author

@marcaaron marcaaron Dec 23, 2020

Choose a reason for hiding this comment

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

If we don't do this we'll allow a request to be made that can't possibly succeed so it's better that we never make it at all.

Copy link
Contributor

@tgolen tgolen Dec 28, 2020

Choose a reason for hiding this comment

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

If someone adds another throw inside enhanceParameters, or there is some kind of bug that gets introduced that throws an unexpected error, it's not clear that this will cause the network queue to be cleared. I'd rather be very explicit about how this work by doing maybe one of these solutions:

  1. Have a method Network.clearRequestQueue() that is called in place of the throw new Error('A request was made without an authToken');. I'd prefer this one because it's very clear what the intention of the code is.
  2. Have specific checking here on err to only clear the request queue when the A request was made without an authToken error is thrown. I don't like this as much because if I were just looking at addAuthTokenToParameters I wouldn't know that clearing the network queue could be a side-effect of throwing an error.

Copy link
Contributor Author

@marcaaron marcaaron Dec 29, 2020

Choose a reason for hiding this comment

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

Ok, that's a valid concern. The problem with 1 if that we probably don't need to make any request if we have no authToken. Clearing the network request queue alone doesn't achieve this since we are already making this request on the next line.

Copy link
Contributor Author

@marcaaron marcaaron Dec 29, 2020

Choose a reason for hiding this comment

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

Will think about this some more.

Copy link
Contributor Author

@marcaaron marcaaron Dec 29, 2020

Choose a reason for hiding this comment

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

Ok I got a better solution. We will:

  1. Clear and pause the request queue via addAuthTokenToParameters()
  2. Check to see if calling enhanceParameters() paused the queue and return early


HttpUtils.xhr(queuedRequest.command, finalParameters, queuedRequest.type)
.then(queuedRequest.resolve)
.catch(queuedRequest.reject);
Expand Down Expand Up @@ -114,9 +120,17 @@ function registerParameterEnhancer(callback) {
enhanceParameters = callback;
}

/**
* Clear the queue so all pending requests will be cancelled
*/
function clearRequestQueue() {
networkRequestQueue = [];
}

export {
post,
pauseRequestQueue,
unpauseRequestQueue,
registerParameterEnhancer,
clearRequestQueue,
};
2 changes: 2 additions & 0 deletions src/libs/NetworkConnection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,9 +97,11 @@ function stopListeningForReconnect() {
clearInterval(sleepTimer);
if (unsubscribeFromNetInfo) {
unsubscribeFromNetInfo();
unsubscribeFromNetInfo = undefined;
}
if (isListeningToAppStateChanges) {
AppState.removeEventListener('change', setAppState);
isListeningToAppStateChanges = false;
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/libs/actions/Report.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -610,7 +610,7 @@ function handleReportChanged(report) {

// A report can be missing a name if a comment is received via pusher event
// and the report does not yet exist in Onyx (eg. a new DM created with the logged in person)
if (report.reportName === undefined) {
if (report.reportID && report.reportName === undefined) {
Copy link
Contributor Author

@marcaaron marcaaron Dec 23, 2020

Choose a reason for hiding this comment

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

In some cases, report data will show up first without a reportID and no reportName. In those cases, we should not attempt to fetch a report that has no reportID

Copy link
Contributor

@Gonals Gonals Dec 24, 2020

Choose a reason for hiding this comment

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

Should we attempt to fetch data with ReportID but undefined reportName?

Copy link
Contributor Author

@marcaaron marcaaron Dec 24, 2020

Choose a reason for hiding this comment

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

Yeah so, it's completely OK to do that (see the comment above) and in fact we want to because we might be handling a report comment from Pusher and we need to download it. Reports that we've downloaded already should have some kind of report name in storage. This is a little weird but it's just how it works for now. Maybe in the future we will fix this to make it more obvious that a report has not been fetched from the server and won't use the reportName implementation detail.

Copy link
Contributor

@tgolen tgolen Dec 28, 2020

Choose a reason for hiding this comment

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

How would it be possible for there to be a report without a reportID? Especially since the Onyx key uses the report ID in the name of the key?

Copy link
Contributor Author

@marcaaron marcaaron Dec 29, 2020

Choose a reason for hiding this comment

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

In theory, this could happen anytime we call merge() for a report that doesn't exist without also including the reportID. I'm not exactly sure what situation leads to that happening here, but this seems like a safe way to avoid calling an API that is guaranteed to fail.

fetchChatReportsByIDs([report.reportID]);
}

Expand Down
8 changes: 6 additions & 2 deletions src/pages/home/report/ReportActionsView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ class ReportActionsView extends React.Component {
this.scrollToListBottom = this.scrollToListBottom.bind(this);
this.recordMaxAction = this.recordMaxAction.bind(this);
this.sortedReportActions = this.updateSortedReportActions();
this.timers = [];

this.state = {
refetchNeeded: true,
Expand All @@ -55,7 +56,7 @@ class ReportActionsView extends React.Component {
componentDidMount() {
this.visibilityChangeEvent = AppState.addEventListener('change', () => {
if (this.props.isActiveReport && Visibility.isVisible()) {
setTimeout(this.recordMaxAction, 3000);
this.timers.push(setTimeout(this.recordMaxAction, 3000));
}
});
if (this.props.isActiveReport) {
Expand Down Expand Up @@ -86,7 +87,7 @@ class ReportActionsView extends React.Component {
// When the number of actions change, wait three seconds, then record the max action
// This will make the unread indicator go away if you receive comments in the same chat you're looking at
if (this.props.isActiveReport && Visibility.isVisible()) {
setTimeout(this.recordMaxAction, 3000);
this.timers.push(setTimeout(this.recordMaxAction, 3000));
}

return;
Expand All @@ -109,9 +110,12 @@ class ReportActionsView extends React.Component {
if (this.keyboardEvent) {
this.keyboardEvent.remove();
}

if (this.visibilityChangeEvent) {
this.visibilityChangeEvent.remove();
}

_.each(this.timers, timer => clearTimeout(timer));
}

/**
Expand Down