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

Revert "Memory management integration with common runtime libraries." #2930

Merged
merged 6 commits into from
Apr 24, 2024

Conversation

SergeyRyabinin
Copy link
Contributor

@SergeyRyabinin SergeyRyabinin commented Apr 17, 2024

Issue #, if available:
CRT STL allocator being used in the SDK.
This forces CRT library to be initialized whenever SDK tries to use a custom allocator.
This prevents SDK from shutting down a CRT logger wrapper after shutting down the CRT library.

Description of changes:
apples and oranges;
separate the sheep from the goats;

Check all that applies:

  • Did a review by yourself.
  • Added proper tests to cover this PR. (If tests are not applicable, explain.)
  • Checked if this PR is a breaking (APIs have been changed) change.
  • Checked if this PR will not introduce cross-platform inconsistent behavior.
  • Checked if this PR would require a ReadMe/Wiki update.

Check which platforms you have built SDK on to verify the correctness of this PR.

  • Linux
  • Windows
  • Android
  • MacOS
  • IOS
  • Other Platforms

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@SergeyRyabinin SergeyRyabinin force-pushed the sr/revertCrtAllocBind branch 3 times, most recently from 07492b7 to 6dd5597 Compare April 18, 2024 18:51
Copy link
Contributor

@jeking3 jeking3 left a comment

Choose a reason for hiding this comment

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

This seems quite reasonable to me. If one does not initialize the SDK one should not be trying to use it.

Copy link
Contributor

@jeking3 jeking3 left a comment

Choose a reason for hiding this comment

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

We should also be initializing the logger and installing it before we initialize the CRT, so we log the CRT startup log messages as well. This will also prove the logger has no dependencies on the CRT, and then the mechanism(s) to cleanly shutdown with delays and quiesces around logging can all be removed.

@@ -210,14 +210,14 @@ namespace Aws

Aws::Config::CleanupConfigAndCredentialsCacheManager();

Aws::Client::CoreErrorsMapper::CleanupCoreErrorsMapper();
Copy link
Contributor

Choose a reason for hiding this comment

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

We should also be initializing the logger and installing it before we initialize the CRT, so we log the CRT startup log messages as well. This will also prove the logger has no dependencies on the CRT, and then the mechanism(s) to cleanly shutdown with delays and quiesces around logging can all be removed.

@SergeyRyabinin SergeyRyabinin marked this pull request as ready for review April 24, 2024 16:21
@SergeyRyabinin SergeyRyabinin merged commit e0e554f into main Apr 24, 2024
4 checks passed
@SergeyRyabinin SergeyRyabinin deleted the sr/revertCrtAllocBind branch April 24, 2024 17:28
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.

4 participants