-
-
Notifications
You must be signed in to change notification settings - Fork 91
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
Fix make_request Retry Error and Prevent 403/429 in Docker #344
base: develop
Are you sure you want to change the base?
Conversation
… matching flights and running fare_checker.
In this case, do we still need the webdriver logic to start the virtual display? I don’t think we should need it as the browser isn’t headed anymore. |
While headless mode avoids the overhead of rendering a GUI, the virtual display adds another layer of compatibility to ensure the environment is closer to a typical user's machine. |
Let me do more testing—let's park this pull request for now. |
@jdholtz please review and let me know thanks. |
@dmytrokoren I just added some more linting rules + changes to the repository to improve the code quality. Do you mind resolving the conflicts? Then I'll take a look. And as always, thanks for the hard work you do to help prevent 403/429 errors! |
other This is an attempt at fixing jdholtz#325 and improving the speed of check-ins. During check-in, what usually happens is that a bad request (400) will go through on the first attempt. This will then take ~3 seconds to finish before the next request is attempted. This change will allow multiple threads to check in at the same time, each starting one second after the other. This will reduce the time between check-in requests and hopefully reduce the total time it takes to check in.
…n-threads' into webdriver-enhacement
Hey @dmytrokoren, the multiple-checkin-threads is not ready to be merged into the develop branch as it hasn't been tested or even known to fix the issue it is intended for. Could you remove it from this PR? Maybe it was accidentally added, but want to make sure. |
Hey @jdholtz, I merged it to test, but I’ll roll it back. I want to try adding a 2-second delay before making the POST request for check-in — I’ll push a new commit for that. |
…ate-limiting Implemented driver.close() to close all browser windows before calling driver.quit() to avoid issues with shutil temporary folder removal
Changed retry attemp to 2 from 1 and decreased retry wait time from 20 to 10 seconds
@jdholtz can we review this PR? |
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.
Looks good overall! Thanks especially for also adding tests.
I wonder if all of the bypasses are needed--filling out the check-in form, the user data dir changes, etc. Have these been verified that detection is much better with all of them, and not just one? Just curious myself, and want to make sure we add what's necessary without extra things that don't help prevent these errors.
Is the user_data_dir persistence needed? The temp dir seems to be removed every time the driver is ran, with the exception of when a driver timeout error is raised--which I think is a bug as there is no quit()
call before raising the timeout error (but I will add it).
@@ -206,6 +206,8 @@ def _check_in_to_flight(self) -> JSON: | |||
site = CHECKIN_URL + self.flight.confirmation_number | |||
|
|||
logger.debug("Making first POST request to check in") | |||
time.sleep(2) |
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.
Let's figure out the delay in #325 instead of adding it here as this has to do more with 400s than 403/429s.
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 want to test how multi-threading performs in real-time. Based on my calculations, Southwest’s API starts accepting check-ins approximately two seconds after the official check-in time.
try: | ||
self._monitor() | ||
except KeyboardInterrupt: | ||
# Add a small delay so the MainThread's message prints first | ||
time.sleep(0.05) | ||
# Lock so all processes are stopped sequentially | ||
with self.lock: | ||
webdriver.reset_temp_dir() |
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.
With an instance of the webdriver class being solely created to reset the temp directory, could we make reset_temp_dir
a static or class method so we don't need to create a new instance that isn't otherwise used? Not very familiar with the classmethod syntax, so let me know if this isn't feasible.
driver.get(CHECKIN_URL) | ||
driver.click_if_visible(".button-popup.confirm-button") | ||
driver.click_if_visible("#onetrust-accept-btn-handler") | ||
|
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.
Can we move the after_page_load.png
screenshot to after these clicks, and then put a screenshot, after_form_submission.png
where the page load screenshot is now? That'll help with specifically debugging this part of the webdriver, if needed.
@@ -284,7 +328,9 @@ def _set_account_name(self, account_monitor: AccountMonitor, response: JSON) -> | |||
) # Don't log as it contains sensitive information | |||
|
|||
def _quit_driver(self, driver: Driver) -> None: | |||
driver.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.
I'm not sure if we need this. .close()
only appears once in the SeleniumBase examples, and that is only after opening a new window to bypass Cloudflare's bot detection, so I think .quit()
is sufficient.
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 noticed that if I don’t call .close() before .quit(), I get an error in the console when deleting the temporary directory.
Seems to resolve the make_request retry error in Docker when fetching matching flights and running fare_checker.