Adding Support for Host Listeners #320

rccarper · 2020-09-17T13:39:36Z

Description of changes:

Adding concept of host listeners to the host resolver. Right now, it only supports listening for new addresses being resolved. Ideally, I would like to add the address-removed/expired callback later.
This PR originally had a way for grabbing all of the currently cached addresses for a host up-front. That will be in a separate, future PR.

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

include/aws/io/host_resolver.h

justinboswell · 2020-10-28T19:51:19Z

source/host_resolver.c

+    aws_host_listener_shutdown_fn *shutdown_callback;
+    void *user_data;
+
+    /* Synchronous data, requires host resolver lock to read/modify*/


Let's put this in its own struct like we usually do, synced_data most likely

The rest of the host resolver doesn't use that paradigm so I was worried it might be confusing to mix that in. Might be okay though!

We should move toward it over time. For this one, I'd rather break consistency and use the right pattern.

justinboswell · 2020-10-28T19:53:40Z

source/host_resolver.c

+    AWS_ASSERT(new_address_list);
+    AWS_ASSERT(listener_list);
+
+    struct aws_linked_list_node *listener_node = aws_linked_list_begin(listener_list);


How do we not need to lock around modifying the listener list?

Because this listener list is owned by the resolver thread, and only the resolver thread ever modifies it.

Ok, can you make that clearer in the comments?

justinboswell · 2020-10-28T19:55:28Z

source/host_resolver.c

+    bool owned_by_resolver_thread;
+    bool pending_destroy;


Let's put these at the end of the structure, and bit pack them

TingDaoK · 2020-10-28T16:51:00Z

include/aws/io/host_resolver.h

+     * struct aws_host_address *host_address = NULL;
+     * aws_array_list_get_at_ptr(new_address_list, (void **)&host_address, address_index);


trivial: It's very nice! But to keep the consistent, we may not need this tip?

I've found getting a value out of an array list can be somewhat confusing. (I usually crash once!) So I liked calling that out here, but maybe it's overkill.

include/aws/io/host_resolver.h

TingDaoK · 2020-10-28T17:53:33Z

source/host_resolver.c

+struct aws_host_listener *aws_host_resolver_add_host_listener(
+    struct aws_host_resolver *resolver,
+    const struct aws_host_listener_options *options) {
+    AWS_ASSERT(resolver);


trivial: AWS_PRECONDITION? Actually, I am not quite clear about the difference between this two macros..

I was trying to keep this consistent with the existing aws_host_resolver functions. :)

TingDaoK · 2020-10-28T17:54:15Z

source/host_resolver.c

+}
+
+int aws_host_resolver_remove_host_listener(struct aws_host_resolver *resolver, struct aws_host_listener *listener) {
+    AWS_ASSERT(resolver);


Same as above!

TingDaoK · 2020-10-28T19:17:59Z

source/host_resolver.c

+    /* Synchronous data, requires host resolver lock to read/modify*/
+    bool owned_by_resolver_thread;
+    bool pending_destroy;
+    struct aws_linked_list_node node;


Is node also synchronous data? And, why don't we use the structure synced_data.

It is, I was trying to keep this consistent with the way the rest of the host_resolver code is written, ie, no synced_data struct. But I'm going to do the synced_data approach.

TingDaoK · 2020-10-28T19:18:50Z

source/host_resolver.c

+/* Structure for holding all listeners for a particular host name. */
+struct host_listener_entry {
+    struct default_host_resolver *resolver;
+    struct aws_linked_list listeners;


maybe comment about what's inside the linked_list? Is that struct host_listener?

TingDaoK · 2020-10-28T19:40:43Z

source/host_resolver.c

+     * Any time the listener list in the listener entry becomes empty, we remove the entry from the table.  This
+     * includes when a resolver thread moves all of the available listeners to its local list.
+     */
+    /* host_name (string) -> host_listener_entry */


Trivial: I feel it might be better to point it out that it stores the pointer of host_listener_entry. But, yeah, basically, it will be only pointers stored in the table. So, ignore this if the other way is more consistent.

I think it's okay but adding the asterisk won't hurt. :)

yeah, because there was a bug in aws-c-mqtt that we stored pointer in array or table, but we forgot it...

TingDaoK · 2020-10-28T20:01:42Z

source/host_resolver.c

+
+/* Notify all listeners with resolve address callbacks, and also clean up any that are waiting to be cleaned up. */
+/* Assumes no lock is held. */
+static void s_resolver_thread_update_listeners(


maybe it will be more clear to comment that the listener_list is a local list, so it doesn't need to be protected by a lock?

…r test.

rccarper requested a review from a team September 17, 2020 13:39

rccarper force-pushed the resolver-listener branch from cece967 to bf8f174 Compare September 17, 2020 13:52

rccarper marked this pull request as draft September 17, 2020 14:42

rccarper force-pushed the resolver-listener branch 2 times, most recently from b328e3e to 2316c79 Compare September 22, 2020 15:26

rccarper marked this pull request as ready for review September 22, 2020 15:42

rccarper changed the title ~~Resolver listener~~ Host Resolver Listener + Get Cached Addresses for Host Sep 22, 2020

graebm reviewed Sep 25, 2020

View reviewed changes

include/aws/io/host_resolver.h Outdated Show resolved Hide resolved

graebm reviewed Sep 25, 2020

View reviewed changes

include/aws/io/host_resolver.h Outdated Show resolved Hide resolved

rccarper marked this pull request as draft September 28, 2020 19:16

rccarper force-pushed the resolver-listener branch 4 times, most recently from 11b90ed to 82a2faa Compare October 23, 2020 19:21

rccarper changed the title ~~Host Resolver Listener + Get Cached Addresses for Host~~ Host Resolver Listener Oct 23, 2020

rccarper changed the title ~~Host Resolver Listener~~ Adding Support for Host Listeners Oct 23, 2020

rccarper marked this pull request as ready for review October 23, 2020 19:52

Adding support for host listeners

01a1628

rccarper force-pushed the resolver-listener branch from 5c171a3 to 01a1628 Compare October 26, 2020 14:32

Merge branch 'master' into resolver-listener

af13cd2

justinboswell reviewed Oct 28, 2020

View reviewed changes

TingDaoK reviewed Oct 28, 2020

View reviewed changes

rccarper added 3 commits November 2, 2020 08:21

PR Feedback

48443be

Fixing thread safety issue with pending_destroy listener flag

c67d1ba

Commenting, logging, and naming changes.

db49aea

rccarper force-pushed the resolver-listener branch from 049c34d to db49aea Compare November 2, 2020 14:41

Fixing CI

781fdbc

rccarper force-pushed the resolver-listener branch from fb736e1 to 781fdbc Compare November 2, 2020 15:12

rccarper mentioned this pull request Nov 2, 2020

Hooking up Host Listener awslabs/aws-c-s3#6

Merged

Fixing comment

f7cb60d

justinboswell approved these changes Nov 2, 2020

View reviewed changes

rccarper added 2 commits November 3, 2020 17:33

Fixing comments, removing unnecessary continue

fdfd665

Possible Clang Fix

a774d4b

rccarper force-pushed the resolver-listener branch from 6bb6c49 to a774d4b Compare November 3, 2020 22:34

rccarper added 4 commits November 3, 2020 17:54

Possible clang fix

fc2af57

Making two local macros for handling synced_data/threaded_data nodes.

61da080

Slightly simplifying resolver thread changes. Adding multiple listene…

7177841

…r test.

Clang format

d775eff

justinboswell approved these changes Nov 4, 2020

View reviewed changes

rccarper merged commit 3153ad4 into master Nov 4, 2020

rccarper deleted the resolver-listener branch November 4, 2020 19:08

graebm mentioned this pull request Mar 28, 2025

Remove unused variables in aws_host_resolver #719

Merged

		* struct aws_host_address *host_address = NULL;
		* aws_array_list_get_at_ptr(new_address_list, (void **)&host_address, address_index);

Adding Support for Host Listeners #320

Adding Support for Host Listeners #320

Uh oh!

Conversation

rccarper commented Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rccarper Oct 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TingDaoK Oct 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rccarper commented Sep 17, 2020 •

edited

Loading

rccarper Oct 28, 2020 •

edited

Loading

TingDaoK Oct 28, 2020 •

edited

Loading