Skip to content

Fix refcount issue #366

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 15 commits into from
Mar 11, 2022
Merged

Fix refcount issue #366

merged 15 commits into from
Mar 11, 2022

Conversation

@TingDaoK
Copy link
Contributor

@TingDaoK TingDaoK commented Mar 10, 2022 •
edited
Loading

Issue #, if available:

  • While connection manager is closing, if the culling task runs, it will increase the refcount back from zero, and decrease it after the task, which will let the clean up happen twice.
  • A bug while acquire the external refcount should NOT acquire the internal refcount.

Description of changes:

  • Culling task hold internal refcount, and cancel the task when the external refcount drops to zero instead of the internal refcount.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

graebm
graebm reviewed Mar 10, 2022
View reviewed changes
source/connection_manager.c
request->error_code = AWS_ERROR_HTTP_CONNECTION_MANAGER_INVALID_STATE_FOR_ACQUIRE;
}
/* It's a use after free crime, we don't want to handle */
AWS_FATAL_ASSERT(manager->state == AWS_HCMST_READY);
Copy link
Contributor

@graebm graebm Mar 10, 2022

Choose a reason for hiding this comment

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

🤣

graebm
graebm approved these changes Mar 11, 2022
View reviewed changes
Copy link
Contributor

@graebm graebm left a comment

Choose a reason for hiding this comment

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

🚀

@TingDaoK TingDaoK merged commit f686b89 into main Mar 11, 2022
@TingDaoK TingDaoK deleted the fix-refcount-issue branch March 11, 2022 00:22
@zoewangg zoewangg mentioned this pull request Mar 11, 2022
Merged
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@graebm graebm graebm approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@TingDaoK @graebm