Skip to content

Conversation

TingDaoK
Copy link
Contributor

Description

Previously, since the underlying CRT client has multiple background threads, there are couple hacks have done to workaround the fork issue. Notes: overall, fork with multi-threading is not encouraged and not well supported.

  1. check the PID to make sure every process creates its own s3 client
  2. If the client was not created by the current process, leave the client in memory to avoid the clean up steps try to invoke background threads already gone.

Well, it works mostly.
CRT has some global state related to threads (There may be more than what I listed, I just listed what I have found so far)

  1. clean up pending threads on fork aws-c-common#1181 for more details
  2. https://github.com/awslabs/aws-c-common/blob/main/source/thread_shared.c#L16, there is a global mutex a background thread can grab when it finises running. So, if the fork happens when one background threads holding the lock, that lock will be a dead lock forever for the subprocess.

Given the findings above, a better workaround is to drop the CRT S3 client and then wait for CRT background threads to join, which will at least get around the two findings listed above. Printout warnings for user about the potential risk.

THIS IS JUST A PROVE OF CONCEPT, feel free to take over the branch 🙇‍♂️

Additional context

  • I have updated the CHANGELOG or README if appropriate

Related items

Testing


By submitting this pull request, I confirm that my contribution is made under the terms of BSD 3-Clause License and I agree to the terms of the LICENSE.

@TingDaoK TingDaoK temporarily deployed to integration-tests March 28, 2025 18:24 — with GitHub Actions Inactive
@TingDaoK TingDaoK temporarily deployed to integration-tests March 28, 2025 18:24 — with GitHub Actions Inactive
@TingDaoK TingDaoK temporarily deployed to integration-tests March 28, 2025 18:24 — with GitHub Actions Inactive
@TingDaoK TingDaoK temporarily deployed to integration-tests March 28, 2025 18:24 — with GitHub Actions Inactive
@TingDaoK TingDaoK temporarily deployed to integration-tests March 28, 2025 18:24 — with GitHub Actions Inactive
@TingDaoK TingDaoK temporarily deployed to integration-tests March 28, 2025 18:24 — with GitHub Actions Inactive
@TingDaoK TingDaoK temporarily deployed to integration-tests March 28, 2025 18:24 — with GitHub Actions Inactive
@TingDaoK TingDaoK temporarily deployed to integration-tests March 28, 2025 18:24 — with GitHub Actions Inactive
@TingDaoK TingDaoK temporarily deployed to integration-tests March 28, 2025 18:24 — with GitHub Actions Inactive
@TingDaoK TingDaoK temporarily deployed to integration-tests March 28, 2025 18:24 — with GitHub Actions Inactive
@TingDaoK TingDaoK temporarily deployed to integration-tests March 28, 2025 18:24 — with GitHub Actions Inactive
@TingDaoK TingDaoK temporarily deployed to integration-tests March 28, 2025 18:24 — with GitHub Actions Inactive
@TingDaoK TingDaoK temporarily deployed to integration-tests March 28, 2025 18:24 — with GitHub Actions Inactive
@TingDaoK TingDaoK temporarily deployed to integration-tests March 28, 2025 18:24 — with GitHub Actions Inactive
@TingDaoK TingDaoK temporarily deployed to integration-tests March 28, 2025 18:24 — with GitHub Actions Inactive
@TingDaoK TingDaoK temporarily deployed to integration-tests March 28, 2025 18:24 — with GitHub Actions Inactive
@TingDaoK TingDaoK temporarily deployed to integration-tests March 28, 2025 18:24 — with GitHub Actions Inactive
@TingDaoK TingDaoK temporarily deployed to integration-tests March 28, 2025 18:24 — with GitHub Actions Inactive
@TingDaoK TingDaoK temporarily deployed to integration-tests March 28, 2025 18:24 — with GitHub Actions Inactive
@TingDaoK TingDaoK temporarily deployed to integration-tests March 28, 2025 18:24 — with GitHub Actions Inactive
Copy link
Contributor

@IsaevIlya IsaevIlya left a comment

Choose a reason for hiding this comment

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

Thank you @TingDaoK for implementing a solution to address the fork compatibility issue in CRT S3 client. These changes should enhance the experience for users who rely on fork functionality.

@IsaevIlya IsaevIlya merged commit bc2271f into awslabs:main Mar 31, 2025
34 checks passed
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.

2 participants