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

✨ Discovery retry support + fixed a race condition in network handling #1529

Merged
merged 13 commits into from
Mar 8, 2024

Conversation

ninadbstack
Copy link
Contributor

@ninadbstack ninadbstack commented Feb 22, 2024

General snapshot discovery retry mechanism
Context:

  • In some cases a page fails to load momentarily, sometimes due to network failure or sometimes due to an unforeseen error, or resource crunch etc
  • In such conditions so far we reported error and dropped that snapshot - which leads to a missing snapshot warning on percy. For some customers this was more common than others.

Changes:

  • We now give an option under discovery section of config to retry discovery. For now its false by default
  • In case the retry is set to true we attempt discovery upto 3 times, printing a log line whenever its retrying the snapshot
  • We can also use this retry option on per snapshot basis by passing {discovery: {retry: true}} in per snapshot options.

Race condition in discovery for certain cases:
Context:

  • Sometimes browser requests same url multiple times in parallel. In such cases we record both as separate requests under #requests but we only get loadingFinished event for one of them. -> other request remains stuck and we throw idle timeout.
  • While debugging i did find that the request is infact stuck in browser - we see it in pending state. But for these requests we in general do get responseReceived callback, but dont get loadingFinished.
  • We need to debug further why the request is getting stuck, doesnt look like its stuck on cli.

Changes:

  • We are now maintaining a finishedUrls set. Whenever we look for idle we now ignore all the urls which are successfully finished at least once before.
  • This is okay to do as in resources we can only attach one response per url and so we dont need to have multiple response objects.

@ninadbstack ninadbstack requested a review from a team as a code owner February 22, 2024 10:55
@ninadbstack ninadbstack force-pushed the race-condition-in-network branch from 46701d8 to d0e18bc Compare March 7, 2024 13:48
@ninadbstack ninadbstack force-pushed the race-condition-in-network branch from d0e18bc to e524b6a Compare March 7, 2024 14:02
@ninadbstack ninadbstack changed the title Race condition in network handling ✨ Discovery retry support + fixed a race condition in network handling Mar 7, 2024
Copy link
Contributor

@prklm10 prklm10 left a comment

Choose a reason for hiding this comment

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

LGTM 🍍

@ninadbstack ninadbstack added the ✨ enhancement New feature or request label Mar 8, 2024
@ninadbstack ninadbstack merged commit d772b43 into master Mar 8, 2024
36 checks passed
@ninadbstack ninadbstack deleted the race-condition-in-network branch March 8, 2024 07:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants