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

bug in retry code for Star Fragment Lamp #2444

Open
jdimpson opened this issue Mar 15, 2023 · 3 comments · May be fixed by #2445
Open

bug in retry code for Star Fragment Lamp #2444

jdimpson opened this issue Mar 15, 2023 · 3 comments · May be fixed by #2445
Assignees

Comments

@jdimpson
Copy link

jdimpson commented Mar 15, 2023

I believe there is a bug in the retry code used get_request() in the Star Fragment Lamp code.

The retry code at line 65 (

) is:
n = ping
This simply assigns the value of ping to the variable n. It does not invoke ping. It should be:
n = ping()

The reason the code appears to work is that the retry code is called like this (on line 78 and at least one other location):
now = get_request(5, io.receive_time())

Specifically, io.recieve_time() is being invoked before get_request(). The result of io.recieve_time() is then being passed into get_request() as the value for ping, which as shown above simply copies that value into n, which it then returns as the result of get_request(). So io.recieve_time() is being invoked outside of the retry code in get_request(), which mean it doesn't benefit from any retries. Instead, get_request() should be invoked like this:
now = get_request(5, io.receive_time)

This passes a reference to the io.recieve_time() as the value of ping, which with the change I suggest above, will then be invoked inside the retry loop in get_request.

(Edit: formatting typo)

jdimpson added a commit to jdimpson/Adafruit_Learning_System_Guides that referenced this issue Mar 15, 2023
@BlitzCityDIY
Copy link
Collaborator

hello- "ping" is the name of the variable here (but perhaps i should change it to avoid confusion). in the function, the request is passed inside the try/except loop, which is nested in the for loop. so it will try making the request the # of times defined as tries. n is the result of the request, in this case either JSON or struct_time. it did retry in my testing.

i did test your suggested fix and it errors out with 'Response' object is not callable and AttributeError: 'bound_method' object has no attribute 'tm_mday'.

@jdimpson
Copy link
Author

The line where get_request(5, io.receive_time()) is being invoked are first invoking io.recieve_time() and sending the resulting time structure as the second argument of get_request(). The second argument, ping then contains the result of io.recieve_time() through out. So it won't ever trigger an exception because all that's happening is the time structure in ping is being assigned to the variablen (unless something went so wrong that assignment fails).

You're seeing the 'Response' object is not callable error because my bug report didn't include the fix for line 87, whih looks like this:
response = get_request(5, requests.get(weather_url))

With the proposed change to get_requests() that activates retries, line 87 needs to change like this:
response = get_request(5, lambda : requests.get(weather_url))

I didn't realize this line was present until I submitted my pull request, which is why I didn't mention it in the bug report.

The lambda expression lets you delay the invocation of requests.get(weather_url) so that it also runs in the retry loop. It's needed because we can't send the bare reference to requests.get like we did for io.recieve_time, because the former needs an argument passed to it. Using the lambda expression lets us embed the argument into a new piece of code that will then be run in a retry loop.

This kind of thing is hard to test because it would require causing io.recieve_time() to fail the first time it is invoked but not the second time.

@jdimpson
Copy link
Author

This might help for anyone who needs a refresher for passing functions as parameters/ variables in python.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants