Skip to content

log errors when project download fails (refs #156) #184

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.

Sign up for GitHub

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

Closed
alexbruy wants to merge 1 commit into from
Closed

log errors when project download fails (refs #156) #184

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion mergin/client_pull.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,9 @@ def download_project_async(mc, project_path, directory, project_version=None):
else:
project_info = latest_proj_info

except ClientError:
except ClientError as e:
mp.log.error("Error while downloading project: " + str(e))
Copy link
Copy Markdown
Contributor

@wonder-sk wonder-sk Oct 12, 2023

Choose a reason for hiding this comment

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

Suggested change
mp.log.error("Error while downloading project: " + str(e))
mp.log.error("Error while querying project info: " + str(e))

mp.log.info("--- download aborted")
_cleanup_failed_download(directory, mp)
raise

Expand Down Expand Up @@ -183,6 +185,8 @@ def download_project_is_running(job):
"""
for future in job.futures:
if future.done() and future.exception() is not None:
job.mp.log.error("Error while downloading project: " + str(future.exception()))
Copy link
Copy Markdown
Contributor

@wonder-sk wonder-sk Oct 12, 2023

Choose a reason for hiding this comment

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

It would be good to produce more details here - including traceback, because it could be arbitrary exception...

job.mp.log.info("--- download aborted")
Copy link
Copy Markdown
Contributor

@wonder-sk wonder-sk Oct 12, 2023

Choose a reason for hiding this comment

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

I am thinking that as the handling of failed futures is the same in _is_running() and in _finalize() methods, maybe it would be good to move it to a new function to avoid code duplication? So here the _is_running() body would become just this:

  1. _abort_download_on_failed_future()
  2. test for future.running()

_cleanup_failed_download(job.directory, job.mp)
raise future.exception()
if future.running():
Expand All @@ -205,6 +209,8 @@ def download_project_finalize(job):
# make sure any exceptions from threads are not lost
for future in job.futures:
if future.exception() is not None:
job.mp.log.error("Error while downloading project: " + str(future.exception()))
job.mp.log.info("--- download aborted")
_cleanup_failed_download(job.directory, job.mp)
raise future.exception()

Expand Down Expand Up @@ -694,6 +700,8 @@ def download_file_finalize(job):
# make sure any exceptions from threads are not lost
for future in job.futures:
if future.exception() is not None:
job.mp.log.error("Error while downloading file: " + str(future.exception()))
job.mp.log.info("--- download aborted")
raise future.exception()

job.mp.log.info("--- download finished")
Expand Down