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 · 14 comments
Open

on_done not called when token has expired #4145

daveisfera opened this issue May 23, 2024 · 14 comments
Labels
documentation This is a problem with documentation. feature-request This issue requests a feature. needs-review 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
@tim-finnigan
Copy link
Contributor

I think we were still waiting on logs where the error referenced here doesn't occur. Going back to the Boto3 events, wouldn't the after-call or after-call-error be what you're looking for?

@tim-finnigan tim-finnigan added the response-requested Waiting on additional information or feedback. label Aug 2, 2024
Copy link

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 Aug 13, 2024
@daveisfera
Copy link
Author

I think we were still waiting on logs where the error referenced here doesn't occur.

I can reproduce the issue with the original example I sent but not the updated one that imports from boto3 rather than s3transfer so I think this was related to that and probably not an actual issue since the original example isn't a supported use case.

Going back to the Boto3 events, wouldn't the after-call or after-call-error be what you're looking for?

It appears that's a general callback and not tied to a specific object/call so is there an example of how to link those calls back to the original call?

Basically, the s3transfer interface appears to be a better/simpler way to handle uploads/downloads in parallel and is documented well enough to get it working. I'm definitely fine with switching to a different interface, but would need guidance on how to do that because I'm not seeing the info in the documentation on how to do that.

@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 Aug 14, 2024
@tim-finnigan tim-finnigan added documentation This is a problem with documentation. feature-request This issue requests a feature. and removed bug This issue is a confirmed bug. labels Aug 14, 2024
@tim-finnigan tim-finnigan removed their assignment Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation This is a problem with documentation. feature-request This issue requests a feature. needs-review p2 This is a standard priority issue s3
Projects
None yet
Development

No branches or pull requests

2 participants