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

Occasionally getting NTPExceptions - uncaught #256

Closed
aaron-pham opened this issue May 2, 2024 · 7 comments
Closed

Occasionally getting NTPExceptions - uncaught #256

aaron-pham opened this issue May 2, 2024 · 7 comments
Labels
bug Something isn't working

Comments

@aaron-pham
Copy link

aaron-pham commented May 2, 2024

Version

7.4

Browser Version

124.0.6367.118

Description

I'm not quite sure why, but locally on my machine I occasionally get a NTPException. I'm not sure what exactly happens (or why), but I notice that during the NTP call we're only catching a socket exception for falling back to system time. So I think we need to catch a more general exception.

To Reproduce

  1. Run script with fare check on

Expected Behavior

If an exception occurs, fall back to system time.

Relevant logs and program output

2024-05-02 10:58:23 DEBUG Process-1[fare_checker:30]: Checking current price for flight
2024-05-02 10:58:23 DEBUG Process-1[fare_checker:88]: Retrieving reservation information
Process Process-1:2:
Traceback (most recent call last):
  File "C:\Users\Aaron\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.10_qbz5n2kfra8p0\LocalCache\local-packages\Python310\site-packages\ntplib.py", line 318, in request
    response_packet, src_addr = s.recvfrom(256)
TimeoutError: timed out

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "C:\Program Files\WindowsApps\PythonSoftwareFoundation.Python.3.10_3.10.3056.0_x64__qbz5n2kfra8p0\lib\multiprocessing\process.py", line 314, in _bootstrap
    self.run()
  File "C:\Program Files\WindowsApps\PythonSoftwareFoundation.Python.3.10_3.10.3056.0_x64__qbz5n2kfra8p0\lib\multiprocessing\process.py", line 108, in run
    self._target(*self._args, **self._kwargs)
  File "C:\seg\auto-southwest-check-in\lib\checkin_handler.py", line 78, in _set_check_in
    self._wait_for_check_in(checkin_time)
  File "C:\seg\auto-southwest-check-in\lib\checkin_handler.py", line 85, in _wait_for_check_in
    current_time = get_current_time()
  File "C:\seg\auto-southwest-check-in\lib\utils.py", line 75, in get_current_time
    response = c.request(NTP_SERVER, version=3)
  File "C:\Users\Aaron\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.10_qbz5n2kfra8p0\LocalCache\local-packages\Python310\site-packages\ntplib.py", line 323, in request
    raise NTPException("No response received from %s." % host)
ntplib.NTPException: No response received from us.pool.ntp.org.
2024-05-02 10:58:25 DEBUG Process-1[utils:42]: Successfully made request after 1 attempts
2024-05-02 10:58:25 DEBUG Process-1[fare_checker:103]: Retrieving search information for the current flight

Additional context

No response

@aaron-pham aaron-pham added the bug Something isn't working label May 2, 2024
jdholtz added a commit that referenced this issue May 2, 2024
Fixes #256. When the request times out, this exception is thrown. For
now, just default to the local time. The timeout parameter in the
request() function of the NTPClient can also be adjusted. This can be
extended for (hopefully) better reliability of the NTP request.
@jdholtz
Copy link
Owner

jdholtz commented May 2, 2024

Thanks for filing this issue. I fixed this in the latest commit of the develop branch.

While falling over to local time is what we want in these scenarios, I still want to see if we can make the NTP requests more reliable (another reason why I don't want to catch a very general exception, as I want to see what exceptions actually could come from this request so it can be looked into and made more reliable, meaning reverting to local time is used as little as possible).

This is where the exception is being raised, which is from a socket timeout. Before merging the latest commit that catches this error, would you be able to set a longer timeout and see if you still get the exception a lot? If you aren't, I can adjust the timeout in the script to make NTP requests more reliable. The change to be done is below:

diff --git a/lib/utils.py b/lib/utils.py
index 77cdb61..9a17a5c 100644
--- a/lib/utils.py
+++ b/lib/utils.py
@@ -72,8 +72,8 @@ def get_current_time() -> datetime:
     c = ntplib.NTPClient()
 
     try:
-        response = c.request(NTP_SERVER, version=3)
+        response = c.request(NTP_SERVER, version=3, timeout=10)
     except socket.gaierror:
         logger.debug("Error requesting time from NTP server. Using local time")
         return datetime.utcnow()

10 seconds seems a bit long, but if the requests do succeed after a longer time, then I see that being a good thing especially because this function is never called when timing is absolutely critical.

@aaron-pham
Copy link
Author

Yeah, let me change the timeout and see how often it happens (if at all).

I am surprised that the default (5 second) timeout is still not long enough sometimes. Makes sense about not catching a more general exception.

@jdholtz
Copy link
Owner

jdholtz commented May 13, 2024

Hey @aaron-pham, have you still been reaching the NTP exceptions with a longer timeout?

@aaron-pham
Copy link
Author

I have not seen the timeouts ever since changing it to 10 seconds. Think we can close this issue now!

@jdholtz
Copy link
Owner

jdholtz commented May 13, 2024

Awesome, I will make the change in the script too. Thanks for following up @aaron-pham!

jdholtz added a commit that referenced this issue May 13, 2024
A follow-up to #256. Setting a longer timeout allows for more time for
NTP requests to go through, making the requests more reliable (the goal
is to use the system's time as little as possible).
jdholtz added a commit that referenced this issue May 13, 2024
A follow-up to #256. Setting a longer timeout allows for more time for
NTP requests to go through, making the requests more reliable (the goal
is to use the system's time as little as possible).
@jdholtz jdholtz closed this as completed May 13, 2024
@aaron-pham
Copy link
Author

Lol of course after I thought it's working good & complete, I have some issues haha.

I'm heading to work so didn't have a chance to debug it at all, but here's the stack trace:

2024-05-15 09:45:27 DEBUG Process-1[utils:78]: Error requesting time from NTP server. Using local time
Process Process-1:
2024-05-15 09:45:29 DEBUG Process-5[reservation_monitor:64]: Lock acquired
2024-05-15 09:45:29 DEBUG Process-5[checkin_scheduler:51]: Refreshing headers for current session
2024-05-15 09:45:29 DEBUG Process-5[webdriver:133]: Starting webdriver for current session (this may take a few minutes)
Traceback (most recent call last):
  File "C:\Program Files\WindowsApps\PythonSoftwareFoundation.Python.3.10_3.10.3056.0_x64__qbz5n2kfra8p0\lib\multiprocessing\process.py", line 314, in _bootstrap
    self.run()
  File "C:\Program Files\WindowsApps\PythonSoftwareFoundation.Python.3.10_3.10.3056.0_x64__qbz5n2kfra8p0\lib\multiprocessing\process.py", line 108, in run
    self._target(*self._args, **self._kwargs)
  File "C:\seg\auto-southwest-check-in\lib\reservation_monitor.py", line 44, in monitor
    self._monitor()
  File "C:\seg\auto-southwest-check-in\lib\reservation_monitor.py", line 174, in _monitor
    self._schedule_reservations(reservations)
  File "C:\seg\auto-southwest-check-in\lib\reservation_monitor.py", line 88, in _schedule_reservations
    self.checkin_scheduler.process_reservations(confirmation_numbers)
  File "C:\seg\auto-southwest-check-in\lib\checkin_scheduler.py", line 42, in process_reservations
    flights.extend(self._get_flights(confirmation_number))
  File "C:\seg\auto-southwest-check-in\lib\checkin_scheduler.py", line 65, in _get_flights
    if flight.departure_time > current_utc_time:
TypeError: can't compare offset-naive and offset-aware datetimes

Let me know if you want me to open a new issue. Also, I probably need to investigate why NTP is timing out so much on my PC. It seems abnormal.

jdholtz added a commit that referenced this issue May 15, 2024
datetime.now(timezone.utc) includes tzinfo whereas datetime.utcnow()
(which is now deprecated so it was changed to the former) does not
include tzinfo. This is a simple fix to just remove the tzinfo as there
is no timezone info for a flight's departure time (when comparing a time
with and without tzinfo, an exception would be thrown).

See #256 (comment)
for a traceback of the issue before this fix.
@jdholtz
Copy link
Owner

jdholtz commented May 15, 2024

Thanks @aaron-pham. When I replaced datetime.utcnow with datetime.now(timezone.utc), I didn't know that the latter had tzinfo which is why this exception was being thrown. I just made the fix and improved a test so this issue will be caught in the future for this scenario.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants