Skip to content

Improve ref count semantics on http message #359

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

Merged
merged 2 commits into from
Jan 21, 2022
Merged

Improve ref count semantics on http message #359

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0baa581
Improve ref count semantics on http message
bretambrose Jan 15, 2022
f20ee3e
Document the changes
bretambrose Jan 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions include/aws/http/request_response.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -615,16 +615,21 @@ struct aws_http_message *aws_http2_message_new_response(struct aws_allocator *al
/**
* Acquire a hold on the object, preventing it from being deleted until
* aws_http_message_release() is called by all those with a hold on it.
*
* This function returns the passed in message (possibly NULL) so that acquire-and-assign can be done with a single
Copy link
Contributor

@TingDaoK TingDaoK Jan 21, 2022

Choose a reason for hiding this comment

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

It feels a program error to acquire on a null message to me.

Copy link
Contributor Author

@bretambrose bretambrose Jan 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Well that's the ref count semantics that I believe we should use and that's the way acquire works in all of the ref count semantics that I have added which is a lot. The point is to make using acquire/release not require conditional logic. If something should be non-null feel free to check it. But acquire/release should not crash and should have the semantics that this uses. Same reasoning as the uh private PR from this afternoon:

acquire_and_assign is a single statement regardless of nullity:

thing->ref = aws_something_acquire(some_ref);

release_and_assign(NULL) is also a single statement, regardless of nullity:

thing->ref = aws_something_release(thing->ref);

We are pushing the conditional checks into acquire/release so that users do not have to do them. If something should be non-null that isn't acquire/releases concern; it should be checked elsewhere. This follows the same reasoning as to why every *_destroy() function written in the last 3-4 years checks for null as the first thing. In that case, it allows your destroy functions to tear down partially built objects (or ones with optional references) without checking null on everything.

* statement.
*/
AWS_HTTP_API
void aws_http_message_acquire(struct aws_http_message *message);
struct aws_http_message *aws_http_message_acquire(struct aws_http_message *message);

/**
* Release a hold on the object.
* The object is deleted when all holds on it are released.
*
* This function always returns NULL so that release-and-assign-NULL can be done with a single statement.
*/
AWS_HTTP_API
void aws_http_message_release(struct aws_http_message *message);
struct aws_http_message *aws_http_message_release(struct aws_http_message *message);

/**
* Deprecated. This is equivalent to aws_http_message_release().
Expand Down
15 changes: 10 additions & 5 deletions source/request_response.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -537,11 +537,11 @@ void aws_http_message_destroy(struct aws_http_message *message) {
aws_http_message_release(message);
}

void aws_http_message_release(struct aws_http_message *message) {
struct aws_http_message *aws_http_message_release(struct aws_http_message *message) {
/* Note that release() may also be used by new() functions to clean up if something goes wrong */
AWS_PRECONDITION(!message || message->allocator);
if (!message) {
return;
return NULL;
}

size_t prev_refcount = aws_atomic_fetch_sub(&message->refcount, 1);
Expand All @@ -557,11 +557,16 @@ void aws_http_message_release(struct aws_http_message *message) {
} else {
AWS_ASSERT(prev_refcount != 0);
}

return NULL;
}

void aws_http_message_acquire(struct aws_http_message *message) {
AWS_PRECONDITION(message);
aws_atomic_fetch_add(&message->refcount, 1);
struct aws_http_message *aws_http_message_acquire(struct aws_http_message *message) {
if (message != NULL) {
aws_atomic_fetch_add(&message->refcount, 1);
}

return message;
}

bool aws_http_message_is_request(const struct aws_http_message *message) {
Expand Down