Apple Network Framework Support #502
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #502 +/- ##
=======================================
Coverage 79.47% 79.48%
=======================================
Files 27 27
Lines 11674 11684 +10
=======================================
+ Hits 9278 9287 +9
- Misses 2396 2397 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
source/connection.c
Outdated
| return server; | ||
|
|
||
| socket_error: | ||
| aws_future_void_release(server_user_data->setup_future); |
There was a problem hiding this comment.
oh man ... this is old-fashioned style CRT error handling, where we'd have different goto labels for each tiny piece that could go wrong. And then try to selectively clean up ONLY the struct members that were successfully initialized.
Please rewrite this to just be 1 goto label that always calls s_http_server_clean_up().
All our misc release() and destroy() and clean_up() functions tolerate being called on something that was never fully initialized. So it's fine to just call s_http_server_clean_up(), and have it naively clean up the mutex and hash table even if they'd failed initialization. It's simpler, and safer than having 2 different places in code that clean up the struct members. As as example of why it's better, observe how these goto labels forgot to clean up some socket members like aws_server_bootstrap_release(server->bootstrap)
There was a problem hiding this comment.
Did not see a better way to determine if we should clean up the mutex, therefore added a flag to indicate if the mutex is inited or not.
source/connection.c
Outdated
| static void s_http_server_clean_up(struct aws_http_server *server, bool cleanup_mutex) { | ||
| if (!server) { | ||
| return; | ||
| } | ||
|
|
||
| aws_server_bootstrap_release(server->bootstrap); | ||
| if (server->bootstrap) { | ||
| aws_server_bootstrap_release(server->bootstrap); | ||
| } |
There was a problem hiding this comment.
you can undo all the changes to this function (except the new call to aws_future_void_release())
We go out of our way to make our clean_up() / destroy() / release() safe to call on NULL or zeroed-out data, so that you can simply always call them
| // if server setup properly, waiting for setup callback | ||
| if (server->socket) { | ||
| aws_future_void_wait(server->setup_future, UINT64_MAX /*timeout*/); | ||
| listen_error = aws_future_void_get_error(server->setup_future); |
There was a problem hiding this comment.
in this path, aws_raise_error() is never called, so the aws_http_server_new() is going to return NULL, but if the user calls aws_last_error() to find out why they'll get 0 or whatever stale old error was on the stack
raise error on listen_error Co-authored-by: Michael Graeb <[email protected]>
Co-authored-by: Michael Graeb <[email protected]>
5 participants
Issue #, if available:
Description of changes:
The consequence changes for Apple Network Framework Support. Depends on awslabs/aws-c-io#662
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.