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

Flag broken updater on every refresh attempt #150

Merged
merged 3 commits into from
Jul 4, 2024

Conversation

hmpf
Copy link
Contributor

@hmpf hmpf commented Jul 3, 2024

@hmpf hmpf requested a review from podliashanyk July 3, 2024 09:48
@hmpf hmpf self-assigned this Jul 3, 2024
Copy link
Contributor

@podliashanyk podliashanyk left a comment

Choose a reason for hiding this comment

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

Both this PR and Uninett/zinolib#71 address only a second part of Uninett/zinolib#68. The first part concerns EventManager and it is not fixed yet. So make sure that merging those two will not close Uninett/zinolib#68.
Also see RFC below

src/howitz/endpoints.py Show resolved Hide resolved
@hmpf
Copy link
Contributor Author

hmpf commented Jul 3, 2024

Both this PR and Uninett/zinolib#71 address only a second part of Uninett/zinolib#68.

This I do not understand.

Issue #68 is two different problems.

@hmpf hmpf requested a review from podliashanyk July 3, 2024 12:36
src/howitz/endpoints.py Show resolved Hide resolved
src/howitz/endpoints.py Outdated Show resolved Hide resolved
src/howitz/endpoints.py Outdated Show resolved Hide resolved
@podliashanyk
Copy link
Contributor

Both this PR and Uninett/zinolib#71 address only a second part of Uninett/zinolib#68.

This I do not understand.

Issue #68 is two different problems.

If you merge this PR then Uninett/zinolib#68 will be automatically closed. See your first comment (PR description) here. Change "Closes #XXX" to "For #XXX" so that GitHub does not close Uninett/zinolib#68 automatically

@hmpf hmpf requested a review from podliashanyk July 4, 2024 07:38
Copy link
Contributor

@podliashanyk podliashanyk left a comment

Choose a reason for hiding this comment

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

Looks much better now, see a question below

Comment on lines 201 to 207
while True:
if current_app.updater is None:
try:
connect_to_updatehandler()
except NotConnectedError as e:
raise LostConnectionError("Could not establish connection to UpdateHandler") from e
updated = current_app.updater.get_event_update()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
while True:
if current_app.updater is None:
try:
connect_to_updatehandler()
except NotConnectedError as e:
raise LostConnectionError("Could not establish connection to UpdateHandler") from e
updated = current_app.updater.get_event_update()
while True:
if current_app.updater is None:
try:
connect_to_updatehandler()
except NotConnectedError as e:
raise LostConnectionError("Could not establish connection to UpdateHandler") from e
updated = current_app.updater.get_event_update()

Is there a specific reason why current_app.updater is None-check should be run on every iteration? Could be moved to before the while-loop instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It used to be before the while loop but that was not sufficient, the connection can be lost during the loop!

@hmpf hmpf merged commit 7310d20 into Uninett:main Jul 4, 2024
5 checks passed
@hmpf hmpf deleted the exception-on-lost-updater branch July 4, 2024 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants