Apple Network Framework Socket Changes #662

sbSteveK · 2024-07-29T17:38:20Z

Apple Network Framework socket integration

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

…windows/iocp

…-c-io into nw_socket

source/darwin/nw_socket.c

bretambrose

Partial initial review. Will do more tomorrow.

bretambrose · 2025-02-12T22:34:01Z

README.md


 Upon a connection being established, the new socket (either as the result of a `connect()` or `start_accept()` call)
 will not be attached to any event loops. It is your responsibility to register it with an event loop to begin receiving
 notifications.

+
+#### V-Table


I still think this does not belong here. Are people supposed to keep this in sync with the real definition? Anyone looking at the code can look at the real definition.

As far as I'm concerned, no vtables should be present in a library README.

source/channel_bootstrap.c

include/aws/io/socket.h

source/darwin/nw_socket.c

…to nw_socket

… nw_socket

source/darwin/nw_socket.c

tests/socket_test.c

bretambrose

Another checkpoint on the overall review. Beyond the comments, I'd love to see regular trace-level logging for every callback function if it does not already have it. We want to be able to follow the functional flow in the logs.

source/darwin/nw_socket.c

bretambrose · 2025-02-18T18:10:45Z

source/darwin/nw_socket.c

+    aws_mem_release(readable_args->allocator, readable_args);
+}
+
+static void s_schedule_on_readable(


I think a better name would be something along the lines of handle_incoming_data. That describes the purpose of the function; the fact that it pushes the data onto a scheduled task is an interior detail.

source/darwin/nw_socket.c

bretambrose · 2025-02-18T18:26:14Z

source/darwin/nw_socket.c

+
+        s_lock_socket_state(nw_socket);
+
+        if (readable_args->is_complete) {


Why wouldn't we condition the whole lock-unlock block on this check? Then we never do the locking until the socket gets closed.

source/darwin/nw_socket.c

bretambrose · 2025-02-18T22:30:49Z

source/darwin/nw_socket.c

+    aws_mem_release(task_args->allocator, task_args);
+}
+
+static void s_schedule_on_listener_success(


s_handle_on_listener_success maybe

bretambrose · 2025-02-18T22:34:03Z

source/darwin/nw_socket.c

+                // released when the connection state changed to nw_connection_state_cancelled
+                s_socket_acquire_internal_ref(new_nw_socket);
+
+                AWS_LOGF_TRACE(


I feel like server connection establishment deserves more than Trace logging. Also, it would be nice if we logged the fact this function was invoked at all. Right now, we're only logging the success path.

bretambrose · 2025-02-18T22:34:47Z

source/darwin/nw_socket.c

+
+        args->nw_socket = nw_socket;
+        args->allocator = nw_socket->allocator;
+        args->error_code = error_code;


Can we convert all Apple error codes to CRT error codes before sticking them in an integer? Maybe log them first before converting? This would apply everywhere we stick an Apple code into an int currently.

If I see error_code in a struct, my expectation is that it's a CRT error code not an OS error code.

source/darwin/nw_socket.c

bretambrose

Not quite finished, but getting close.

Would suggest doing a pass on log levels and ensuring that they make sense.
Trace - most appropriate for data processing step and extremely common/spammy events
Debug/Info - appropriate for connection-level events, state changes, etc...

bretambrose · 2025-02-19T18:04:56Z

source/darwin/nw_socket.c

+    AWS_LOGF_DEBUG(
+        AWS_LS_IO_SOCKET, "id=%p handle=%p: beginning connect.", (void *)socket, socket->io_handle.data.handle);
+
+    if (socket->event_loop) {


Given the previous fatal assert, this can be removed

bretambrose · 2025-02-19T18:16:57Z

source/darwin/nw_socket.c

+    // "connect" action after aws_socket_init() regardless it's a UDP socket or a TCP socket.
+    AWS_FATAL_ASSERT(on_connection_result);
+    s_lock_socket_state(nw_socket);
+    if (nw_socket->synced_state.state != INIT) {


Unless you change the state here as well, this isn't at all concurrent-safe. Maybe our usage of aws_socket keeps this from being an issue but it's really uncomfortable having "this-is-only-safe-because-this-is-an-internal-API-and-our-internal-usage-just-happens-to-not-blow-it-up" blocks throughout the implementation.

bretambrose · 2025-02-19T18:36:56Z

source/darwin/nw_socket.c

+        return AWS_OP_ERR;
+    }
+
+    struct socket_address address;


We have a CRT-defined union type (which I don't think is needed) and then we pass it to a system call (nw_endpoint_create_address). That's not something we want to ever do even if the types currently match.

bretambrose · 2025-02-19T18:40:51Z

source/darwin/nw_socket.c

+    struct socket_address address;
+    AWS_ZERO_STRUCT(address);
+    int pton_err = 1;
+    if (socket->options.domain == AWS_SOCKET_IPV4) {


better as a switch statement maybe

bretambrose · 2025-02-19T18:41:48Z

source/darwin/nw_socket.c

+        address.sock_addr_types.un_addr.sun_len = sizeof(struct sockaddr_un);
+
+    } else {
+        AWS_FATAL_ASSERT(0);


Why is it a fatal assert? This is user-controlled; should passing in an unknown domain cause things to crash?

bretambrose · 2025-02-19T21:52:31Z

source/darwin/nw_socket.c

+            (void *)nw_socket,
+            (void *)nw_socket->os_handle.nw_connection);
+
+        s_schedule_next_read(nw_socket);


Why is this necessary given that the read callback always tries to schedule a new one?

source/darwin/nw_socket.c

bretambrose · 2025-02-19T21:57:40Z

source/darwin/nw_socket.c

+    }
+
+    struct nw_socket *nw_socket = socket->impl;
+    if (!(nw_socket->synced_state.state & CONNECTED_WRITE)) {


How is this safe? Are we asserting that only the event loop ever modifies this?

bretambrose · 2025-02-19T21:58:55Z

source/darwin/nw_socket.c

+
+    AWS_FATAL_ASSERT(written_fn);
+
+    dispatch_data_t data = dispatch_data_create(cursor->ptr, cursor->len, NULL, DISPATCH_DATA_DESTRUCTOR_DEFAULT);


Can this fail? I don't think we should assume it can't. Unlike our stuff, the OS isn't going to intentionally crash if resource allocation fails.

bretambrose · 2025-02-19T23:03:58Z

source/darwin/nw_socket.c

+
+          if (error_code) {
+              nw_socket->last_error = error_code;
+              aws_raise_error(error_code);


Probably not useful to raise error here in a callback

bretambrose

Looked through channel bootstrap.

Would really like to see _new, _destroy functions for all the task-arg situations across the whole PR.

bretambrose · 2025-02-20T18:25:58Z

include/aws/io/channel_bootstrap.h

@@ -296,6 +305,9 @@ AWS_IO_API int aws_server_bootstrap_set_alpn_callback(
 AWS_IO_API struct aws_socket *aws_server_bootstrap_new_socket_listener(
    const struct aws_server_socket_channel_bootstrap_options *bootstrap_options);

+AWS_IO_API struct aws_socket *aws_server_bootstrap_new_socket_listener_async(


Can we just make all listener implementations use an async path?

bretambrose · 2025-02-20T18:29:54Z

source/channel_bootstrap.c


+        struct socket_shutdown_setup_channel_args *close_args =


Let's not repeat this and instead add _new and _destroy functions for socket_shutdown_setup_channel_args

bretambrose · 2025-02-20T18:32:51Z

source/channel_bootstrap.c

@@ -607,14 +662,19 @@ static void s_on_client_connection_established(struct aws_socket *socket, int er
    connection_args->channel_data.channel = aws_channel_new(connection_args->bootstrap->allocator, &args);

    if (!connection_args->channel_data.channel) {
+        struct socket_shutdown_setup_channel_args *close_args =


use _new here too

bretambrose · 2025-02-20T19:00:19Z

source/channel_bootstrap.c

+    aws_mem_release(shutdown_args->allocator, shutdown_args);
+}
+
+static void s_socket_shutdown_complete_setup_connection_args_no_release_fn(void *user_data) {


Seems like it would be simpler just to add a bool controlling release in the args structure rather than duplicating a function

bretambrose · 2025-02-20T21:02:59Z

source/channel_bootstrap.c

@@ -1402,12 +1606,20 @@ void s_on_server_connection_result(

 error_cleanup:
    /* no channel is created */
-    connection_args->incoming_callback(connection_args->bootstrap, aws_last_error(), NULL, connection_args->user_data);
-
+    (void)socket; // to avoid expression error after a label


can just use a semicolon

bretambrose · 2025-02-20T21:03:31Z

source/channel_bootstrap.c

    struct aws_allocator *allocator = new_socket->allocator;
+
+    struct socket_shutdown_server_connection_result_args *close_args =


I see this pattern everywhere. Can we create some helper functions and use them instead?

sbSteveK · 2025-02-21T19:11:18Z

source/darwin/nw_socket.c

+    size_t current_offset;
+};
+
+static void s_destroy_read_queue_node(struct read_queue_node *node) {


Trivial: naming convention on destroy functions. This should be renamed to s_read_queue_node_destroy(). Also, if we have a destroy function for read_queue_node we should probably set up its pair of s_read_queue_node_new() that is called when we are creating something that needs to be destroyed. It looks like there's only one place where this node is created but it feels right to have them both.

sbSteveK · 2025-02-21T23:23:09Z

source/darwin/nw_socket.c

+                memcpy(socket->local_endpoint.address, hostname, to_copy);
+                socket->local_endpoint.port = port;
+            }
+            nw_release(local_endpoint);


The local_endpoint doesn't appear to be released if there's no socket. We also appear to not use the hostname and port members if there's no socket. We could probably move hostname and port into this if block and move the nw_release of local_endpoint outside of it unless it's being taken care of elsewhere.

source/darwin/nw_socket.c

sbSteveK and others added 30 commits July 29, 2024 10:37

socket related from network_framework_integration branch

14a3386

Merge branch 'grand_dispatch_queue' into nw_socket

9715a3e

missed s_socket_listen

b830a86

move aws_socket_init_poll_based platform not supported function into …

cccbda2

…windows/iocp

small cleanups/comments

caac9a5

Merge branch 'grand_dispatch_queue' into nw_socket

eb59ff1

nw_socket.c changes

8a794de

add nw_connection_t to nw_socket

553d45f

read from socket works

cf610e9

remove prints

07bac64

trivial edits

dd1fbf2

check correct vtable func

2fd514c

clang format

2a0da42

socket_test add a manual way to set event_loop_style in options

9cc6620

event_loop add undefined event loop style and clang format

e3281ee

clang format

43fd436

event_loop.c clang formatting and configurations

1c1cd02

formatting

88f6de3

format

cf53cc6

macos errors

a7ab224

fix test

62fd06d

formatting

cbb8c42

test fix

4ce33ee

prototype void

29ab896

fix style func

f9cd5d0

sprintf -> snprintf

2e55d2d

manual default change for testing

6938bc3

Merge branch 'grand_dispatch_queue' into nw_socket

4658492

Merge branch 'grand_dispatch_queue' of https://github.com/awslabs/aws…

ef8d53f

…-c-io into nw_socket

setup connection timeout

731ba49

xiazhvera added 2 commits February 13, 2025 10:01

use is_complete to close the socket

8dc88fd

add prints to trace read queue

83981c3

sfod reviewed Feb 13, 2025

View reviewed changes

xiazhvera added 5 commits February 14, 2025 14:25

try fix processing read data on error

7dec84e

WIP: do not cancel connection before write finished

bdcd5ee

WIP: do not cancel connection before write finished

b5a0e85

WIP DEBUG read operation on closing

2d312da

clean up socket

6043ce4

bretambrose reviewed Feb 17, 2025

View reviewed changes

xiazhvera added 5 commits February 17, 2025 17:23

update code review

2f7eec7

Merge branch 'grand_dispatch_queue' of github.com:awslabs/aws-c-io in…

043b468

…to nw_socket

Merge branch 'nw_socket_shutdown' of github.com:awslabs/aws-c-io into…

5ebcf77

… nw_socket

fix merge

17d2190

generaize aws_socket_start_accept api

4b1a553

sfod reviewed Feb 18, 2025

View reviewed changes

source/darwin/nw_socket.c Outdated Show resolved Hide resolved

tests/socket_test.c Outdated Show resolved Hide resolved

improve socket state setup

b86e381

bretambrose reviewed Feb 18, 2025

View reviewed changes

xiazhvera added 5 commits February 18, 2025 16:03

improve task mem allocation

098b1bd

fix socket_cancel and windows

c973549

make sure nw_socket close has an event loop

0368bd9

more code review feedback

304f588

fix windows socket compilation

214913a

bretambrose reviewed Feb 19, 2025

View reviewed changes

bretambrose reviewed Feb 20, 2025

View reviewed changes

sbSteveK commented Feb 21, 2025

View reviewed changes

source/darwin/nw_socket.c Outdated Show resolved Hide resolved

xiazhvera added 3 commits February 24, 2025 10:41

fix race condition for releasing socket

031a0e0

[WIP] Test nw_socket with downstream (#711)

fbcafd8

rename locks...

f4e37dc

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apple Network Framework Socket Changes #662

Apple Network Framework Socket Changes #662

sbSteveK commented Jul 29, 2024

bretambrose left a comment

bretambrose Feb 12, 2025

bretambrose left a comment

bretambrose Feb 18, 2025

bretambrose Feb 18, 2025

bretambrose Feb 18, 2025

bretambrose Feb 18, 2025

bretambrose Feb 18, 2025

bretambrose left a comment

bretambrose Feb 19, 2025

bretambrose Feb 19, 2025

bretambrose Feb 19, 2025

bretambrose Feb 19, 2025

bretambrose Feb 19, 2025

bretambrose Feb 19, 2025

bretambrose Feb 19, 2025

bretambrose Feb 19, 2025

bretambrose Feb 19, 2025

bretambrose left a comment

bretambrose Feb 20, 2025

bretambrose Feb 20, 2025

bretambrose Feb 20, 2025

bretambrose Feb 20, 2025

bretambrose Feb 20, 2025

bretambrose Feb 20, 2025

sbSteveK Feb 21, 2025

sbSteveK Feb 21, 2025


		s_lock_socket_state(nw_socket);

		if (readable_args->is_complete) {


		AWS_FATAL_ASSERT(written_fn);

		dispatch_data_t data = dispatch_data_create(cursor->ptr, cursor->len, NULL, DISPATCH_DATA_DESTRUCTOR_DEFAULT);

		struct aws_allocator *allocator = new_socket->allocator;

		struct socket_shutdown_server_connection_result_args *close_args =

Apple Network Framework Socket Changes #662

Are you sure you want to change the base?

Apple Network Framework Socket Changes #662

Conversation

sbSteveK commented Jul 29, 2024

bretambrose left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bretambrose left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bretambrose left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bretambrose left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment