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

on_done not called when token has expired #4145

Open
daveisfera opened this issue May 23, 2024 · 11 comments
Open

on_done not called when token has expired #4145

daveisfera opened this issue May 23, 2024 · 11 comments
Assignees
Labels
bug This issue is a confirmed bug. p2 This is a standard priority issue s3

Comments

@daveisfera
Copy link

Describe the bug

If the token has expired, then the file isn't uploaded (expected), but on_done is not called and there's no way to get notification of the failure

(refiling boto/s3transfer#304 here since I use s3transfer through boto3)

Expected Behavior

An error would be reported

Current Behavior

The upload silently fails

Reproduction Steps

  1. Create a token using aws-vault
  2. Do an upload
  3. Verify success
  4. Wait for token to expire
  5. Do an upload
  6. Notice that no error is reported and on_done is never called

Possible Solution

Report an error using on_done in the same way from before the expiration

Additional Information/Context

No response

SDK version used

1.34.97

Environment details (OS name and version, etc.)

Debian Bookworm with Python 3.12.3

@daveisfera daveisfera added bug This issue is a confirmed bug. needs-triage This issue or PR still needs to be triaged. labels May 23, 2024
@tim-finnigan tim-finnigan self-assigned this Jun 4, 2024
@tim-finnigan
Copy link
Contributor

Thanks for reaching out. Unfortunately we cannot guarantee compatibility with third-party libraries like aws-vault. To investigate a boto3 issue here we would need a code snippet to reproduce the issue, in addition to debug logs (with sensitive info redacted) which you can get by adding boto3.set_stream_logger('') to your script.

@tim-finnigan tim-finnigan added third-party closing-soon This issue will automatically close in 4 days unless further comments are made. p2 This is a standard priority issue and removed needs-triage This issue or PR still needs to be triaged. labels Jun 4, 2024
@daveisfera
Copy link
Author

aws-vault is just a convenient way to generate the necessary ENVs to authorizing with boto3 and the same problem can be reproduced with any token that expires. I'll grab logs of it happening.

@github-actions github-actions bot removed the closing-soon This issue will automatically close in 4 days unless further comments are made. label Jun 6, 2024
@daveisfera
Copy link
Author

Here's a minimal reproducer (and I believe the simplest example of using BaseSubscriber and on_done that's possible) and I've attached the logs from running it with an expired token:

import logging
from io import BytesIO

from boto3 import set_stream_logger
from boto3.s3.transfer import BaseSubscriber
from boto3.session import Session
from s3transfer.manager import TransferConfig, TransferManager

logger = logging.getLogger(__name__)
logging.basicConfig(level=logging.INFO)

set_stream_logger("")


class CheckDone(BaseSubscriber):
    def on_done(self, future, **kwargs) -> None:
        try:
            res = future.result()
            logger.info("Upload worked: %s", res)
        except Exception as e:
            logger.error("Upload failed: %s", e)


def _main() -> None:
    session = Session()
    s3_tm = TransferManager(session.client("s3"), TransferConfig(max_request_concurrency=3))
    s3_tm.upload(
        BytesIO(b"testing"),
        "test_bucket",
        "s3transfer_test.txt",
        subscribers=[CheckDone("s3transfer_test.txt")],
    )
    s3_tm.shutdown()
    logger.info("Done")


if __name__ == "__main__":
    _main()

s3transfer_test.log

@tim-finnigan
Copy link
Contributor

Thanks for following up. We don't recommend using s3transfer directly, as noted in the README:

This project is not currently GA. If you are planning to use this code in production, make sure to lock to a minor version as interfaces may break from minor version to minor version. For a basic, stable interface of s3transfer, try the interfaces exposed in boto3

Have you tried using the Boto3 upload methods for S3? You can handle any errors as documented here or handle events like after-call-error.

@tim-finnigan tim-finnigan added response-requested Waiting on additional information or feedback. s3 and removed third-party labels Jun 20, 2024
@daveisfera
Copy link
Author

Sorry, I pulled from the wrong import when putting together that minimal reproducer. Here's it with the import from boto3:

import logging
from io import BytesIO

from boto3 import set_stream_logger
from boto3.s3.transfer import BaseSubscriber, TransferConfig, TransferManager
from boto3.session import Session

logger = logging.getLogger(__name__)
logging.basicConfig(level=logging.INFO)

set_stream_logger("")


class CheckDone(BaseSubscriber):
    def on_done(self, future, **kwargs) -> None:
        try:
            res = future.result()
            logger.info("Upload worked: %s", res)
        except Exception as e:
            logger.error("Upload failed: %s", e)


def _main() -> None:
    session = Session()
    s3_tm = TransferManager(session.client("s3"), TransferConfig(max_concurrency=3))
    s3_tm.upload(
        BytesIO(b"testing"),
        "test_bucket",
        "s3transfer_test.txt",
        subscribers=[CheckDone("s3transfer_test.txt")],
    )
    s3_tm.shutdown()
    logger.info("Done")


if __name__ == "__main__":
    _main()
@github-actions github-actions bot removed the response-requested Waiting on additional information or feedback. label Jun 21, 2024
@tim-finnigan
Copy link
Contributor

That snippet is still using the TransferManager directly which isn't recommended:

Thanks for following up. We don't recommend using s3transfer directly, as noted in the README:

This project is not currently GA. If you are planning to use this code in production, make sure to lock to a minor version as interfaces may break from minor version to minor version. For a basic, stable interface of s3transfer, try the interfaces exposed in boto3

Have you tried using the Boto3 upload methods for S3? You can handle any errors as documented here or handle events like after-call-error.

@daveisfera
Copy link
Author

Then what is the recommended way to upload multiple files in parallel in a separate thread using boto3?

This is the only information I could find about uploading in parallel and it looks like I'm doing a simpler version of what that code shows, so is there a better way?

@tim-finnigan
Copy link
Contributor

Hi @daveisfera — we recommend using upload_file or upload_fileobj. And here is the documentation on multithreading/multiprocessing. Although the S3Transfer code you shared is public it's still recommended to use Boto3 directly.

Also looking back at the logs you shared, it looks like you're on_done function is being called and the exception is logged (ERROR:__main__:Upload failed:...).

@tim-finnigan tim-finnigan added the response-requested Waiting on additional information or feedback. label Jun 25, 2024
@daveisfera
Copy link
Author

Hi @daveisfera — we recommend using upload_file or upload_fileobj. And here is the documentation on multithreading/multiprocessing. Although the S3Transfer code you shared is public it's still recommended to use Boto3 directly.

I'm glad to switch but that documentation doesn't seem to provide information on how to actually perform the uploads in another thread. It also mentions events but I don't see how to receive an event notification like on_done, so is there detailed information on how to use it?

Also looking back at the logs you shared, it looks like you're on_done function is being called and the exception is logged (ERROR:__main__:Upload failed:...).

You are correct. I'll check and see if I can get logs from when the callback doesn't happen

@github-actions github-actions bot removed the response-requested Waiting on additional information or feedback. label Jun 26, 2024
@tim-finnigan tim-finnigan added the response-requested Waiting on additional information or feedback. label Jul 2, 2024
Copy link

github-actions bot commented Jul 6, 2024

Greetings! It looks like this issue hasn’t been active in longer than five days. We encourage you to check if this is still an issue in the latest release. In the absence of more information, we will be closing this issue soon. If you find that this is still a problem, please feel free to provide a comment or upvote with a reaction on the initial post to prevent automatic closure. If the issue is already closed, please feel free to open a new one.

@github-actions github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Jul 6, 2024
@daveisfera
Copy link
Author

Still waiting for guidance on how to do the upload in parallel and receive notifications of when it completes or has an error

@github-actions github-actions bot removed closing-soon This issue will automatically close in 4 days unless further comments are made. response-requested Waiting on additional information or feedback. labels Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a confirmed bug. p2 This is a standard priority issue s3
2 participants