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

Errors not propagated when upload_file fails #89

Open
fidel-perez opened this issue Dec 12, 2022 · 3 comments
Open

Errors not propagated when upload_file fails #89

fidel-perez opened this issue Dec 12, 2022 · 3 comments

Comments

@fidel-perez
Copy link

fidel-perez commented Dec 12, 2022

When s3.py -> upload_file fails, the errors are not propagated because of concurrent execution.
After reviewing the last repository changes, I believe this bug was introduced in the latest PR that was merged.

How to reproduce

  • Install following the readme instructions
  • Configure pointing towards an S3 bucket using wrong credentials or wrong URL or any other wrong configuration
  • Run the target using a dummy input tap

You should get no errors, despite no file was uploaded to S3 (because of the miss configuration done on purpose).

I found the problem was in the upload_file method after trying adding this try/except to the code and finally being able to see where it was failing by using the print statement:

try:
    result = config['client'].upload_file(
        file_metadata['absolute_path'].as_posix(),
        config.get('s3_bucket'),
        file_metadata['relative_path'],
        **encryption_args)
except Exception as e:
    print(e)
@fidel-perez
Copy link
Author

fidel-perez commented Dec 12, 2022

I just made some tests locally using the commit previous to the mentioned PR and can confirm the bug was introduced on that merge.

@ome9ax
Copy link
Owner

ome9ax commented Apr 9, 2023

Hey @fidel-perez I've added a little change in the main branch to disable the ThreadPoolExecutor with the config option "thread_pool": false. Please let me know if that resolves the issue?

@fidel-perez
Copy link
Author

Thank you for this. We stopped focusing on this repo in my job so I think we can close the issue with your recent fix!
I would recommend adding one line or two about this in the readme though :)

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

No branches or pull requests

2 participants