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

Fix hang on shutdown with multiple threadpools #266

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

TedSinger
Copy link

This patch fixes an issue that occurs when a download completion is expected after the threadpools have shutdown.

Here is a test that reproduces the problem. Without the patch, running this hangs forever, demonstrating the bug. With the patch, pytest terminates as soon as the KeyboardInterrupt is thrown, as expected. This test is not appropriate to include in the test suite, since it halts the whole test runner on success and hangs on failure.
(Any error lesser than KeyboardInterrupt fails to demonstrate the bug under pytest. pytest catches the error and prevents the global shutdown hook from triggering until the tests are cleaned up)

    # in tests.integration.test_download.TestDownload
    def test_concurrent_shutdown_in_exception(self):
        import io
        from concurrent.futures import ThreadPoolExecutor

        names = ['foo.txt', 'bar.txt']
        for name in names:
            filename = self.files.create_file_with_size(
                name, filesize=750 * 1024 * 1024
            )
            self.upload_file(filename name)

        def download(key):
            # This is how a boto3 client implements `download_fileobj`
            with self.create_transfer_manager(self.config) as manager:
                future = manager.download(
                    bucket=self.bucket_name,
                    key=key
                    fileobj=io.BytesIO()
                )
                return future.result()

        mapper = ThreadPoolExecutor(max_workers=2)
        iterable = mapper.map(download, names)
        try:
            # Suppose some unrelated error causes Python to try to shutdown...
            raise KeyboardInterrupt()
        finally:
            # Because we are running things concurrently, some other threadpool
            # demands a new result, and we wait on an already started future. That
            # should be fine, as the future should either finish or crash.
            # But s3transfer.futures.TransferCoordinator fails to call self.announce_done()
            # and hangs forever
            next(iterable)

This patch is licensed Apache 2.0, in accordance with the boto/s3transfer contribution policy.

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

Successfully merging this pull request may close these issues.

1 participant