Current design seems to expose a function that searches for or adds a target (i.e. get_or_add_target) and passes the address of the target to the function that actually connects to the server.

The approach seem to have two issues:

• once allocated, pool->targets becomes unexpandable (or shrinkable), unless the mutex is locked from before the call to get_or_add_target to after the call to connect_to_target. Otherwise, we would have a race condition.
• a target entry would be created for a server that always sends connection: close, which is IMO a waste

So you might want to consider the following:

• do not expose get_or_add_target
• accept the (host, port, is_ssl) triplet as an argument to connect_to_target
• the function checks if a matching target exists, and if a target with a pooled socket found, returns the socket
• otherwise, the socket pool connects to a new socket (note that you do not need to ensure that a matching target exists)
• in h2o_socketpool_return, look for a matching target, and if not found, create a new target. Add to the target the socket being returned. You might need to pass the triplet as arguments to the function.

By using this approach, all locking operations can be kept within the socketpool.c. Also, pool->targets becomes not only expandable but also shrinkable (i.e. targets without any poold sockets can be garbage-collected).

I believe that there is no reason to delay the instantiation of the pool, now that it can hold any number of targets. Let's have it initialized in src/main.c.

How about using words: specific and global instead of (static and dynamic)?

When the PROXY protocol is being used (i.e. when overrides->use_proxy_protocol is set), the client needs to establish a new connection even when there is a pooled connection to the peer, since the header (as defined in https://www.haproxy.org/download/1.8/doc/proxy-protocol.txt) needs to be sent at the beginning of the connection.

However, it seems that h2o_http1client_connect do not provide an interface to enforce estabilishing a new connection. In other words, there is a chance that we would use a pool connection here.

OTOH, I can understand that the probability of seeing such issue is rare; it would only happen if the same destination identified by its host:port is accessed in both ways (i.e. in one path with use_proxy_protocol set and in the other path without).

So I do not think that it is absolutely necessary to fix the issue.

However, I would appreciate it if you could at least leave a comment here that such possibility exists, possibly by copy-pasting this comment to the source code.

How about adding functions to url.h / url.c? It could be something like h2o_url_init_with_hostport and h2o_url_init_with_sunpath.

I would appreciate it if you could rename link to all_link so that the roles of the two links (the other is target_link) become clearer.

Can't this be changed to something like:

h2o_multithread_receiver_t *getaddr_receiver;
size_t selected_target;
struct {
    int *tried;
    size_t try_count;
} lb;

What I am suggesting here is that you only need to retain the selected index that designates the target within pool->targets. I believe that not using unions would reduce the impact on the code-base.

Note that the proposed struct removes lb.targets. I believe that it is possible; please see 5780763.

Can't we define the global socket pool as a global property of h2o_globalconf_t?

For libh2o, it would be beneficial to be able to release all sockets when a global configuration gets disposed (current approach does not do that). I believe that the code would become simpler, since we can eliminate the set_default_sockpool (and the get) function.

No. I think that you should move the call to lookup_target outside of the critical section.