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

Attempt to prevent reload loops #22529

Merged
merged 3 commits into from
Jul 20, 2022
Merged

Attempt to prevent reload loops #22529

merged 3 commits into from
Jul 20, 2022

Conversation

alexmv
Copy link
Collaborator

@alexmv alexmv commented Jul 19, 2022

Self-review checklist

  • Self-reviewed the changes for clarity and maintainability
    (variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

  • Explains differences from previous plans (e.g., issue description).
  • Highlights technical choices and bugs encountered.
  • Calls out remaining decisions and concerns.
  • Automated tests verify logic where appropriate.

Individual commits are ready for review (see commit discipline).

  • Each commit is a coherent idea.
  • Commit message(s) explain reasoning and motivation for changes.

Completed manual review and testing of the following:

  • Visual appearance of the changes.
  • Responsiveness and internationalization.
  • Strings and tooltips.
  • End-to-end functionality of buttons, interactions and flows.
  • Corner cases, error conditions, and easily imagined bugs.

alexmv added 3 commits July 19, 2022 14:09
A `reload.initiate({immediate: true, ...})` *should* not return, as it
should trigger a `window.location.reload` and stop execution.

In the event that it continues execution and returns (for instance,
due to being in the background and reloads being suppressed for
power-saving -- see zulip#6821), there is no need to fall through and
potentially schedule a 90-second-later retry.
Prevent a non-immediate reload from being scheduled while an immediate
reload is already in progress.  This is highly unlikely in practice,
but is a reasonable safeguard.
We have observed infrequent storms of accesses (tens of thousands of
requests to minute) to `/` after an event queue expires.  The current
best theory is that the act of reloading the page itself triggers a
focus event, which itself triggers a reload before the prior one had
had time to do anything but send the network request.

Since the `focus` event here is merely as a backstop in case the
synchronous reloading and deferred reloading fail, we need only run it
once.
@alexmv alexmv marked this pull request as ready for review July 19, 2022 22:52
@timabbott timabbott merged commit 2d21836 into zulip:main Jul 20, 2022
@timabbott
Copy link
Member

Merged, thanks @alexmv!

@alexmv
Copy link
Collaborator Author

alexmv commented Jul 20, 2022

We may want to backport this if it seems to work. I'm going to tag it as such for now, and we can decide later if we want to skip it.

@alexmv alexmv added the backport candidate Items we should consider backporting to the bug fix release series. label Jul 20, 2022
@timabbott
Copy link
Member

I'm skipping backporting this until #22721 is resolved, just in case the changes in this PR are related to that report.

@alexmv
Copy link
Collaborator Author

alexmv commented Oct 3, 2022

#22721 (comment) seems to confirm that this is not a recent regression; is there any other reason to not backport this?

@timabbott
Copy link
Member

No, we can do so now.

@alexmv alexmv mentioned this pull request Oct 11, 2022
12 tasks
@alexmv
Copy link
Collaborator Author

alexmv commented Oct 11, 2022

Opened #23199 for the backport.

@alexmv alexmv deleted the reload-loop branch October 11, 2022 18:21
@alexmv alexmv removed the backport candidate Items we should consider backporting to the bug fix release series. label Oct 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants