Start scaffolding some of the types and APIs

[jhump]

This is hopefully a reasonable starting point to discuss the basic design of the client/transport that this repo will provide. This proposes using a hierarchy of transports that is three levels deep.

Most of the "real" stuff we want to do is all TODO. This has a very basic NewClient API that accepts options (no options defined yet...). The options would be how users configure the client, providing custom resolvers, balancers, health-checkers, etc...

This also creates a very rudimentary implementation (more of an outline really) of the main two levels of the transport hierarchy: the main transport, and a "connection pool" transport that manages requests for a single target domain.

This will all have to be refactored as features are implemented. None of the transport implementation needs to be considered "real": this was mostly to get a conversation started about the architecture so far.

---

I think design question number 1 is: "Should this provide a complete HTTP client, usable for requests to any/all domains? Or should it instead provide a client for a particular service, which returns errors if used with requests to the wrong domain?"

What's in this branch is the former, thus necessitating the "top-level" transport. But configuration for some of the stuff we're talking about is likely to often be "per service". So trying to provide a "panacea" client, all in one instance, could mean either complicated configuration (to expose ability to configure certain things per domain) or error-prone to use (e.g. user creates two clients with different configs but then uses the wrong one with a particular HTTP request).

We could pivot to the latter approach, where you instead have something akin to a Dial function, that returns a client that is usable just for the given host/domain. That way, there's no need to support multiple configurations (based on target domain) in a single client, and we can even fail-fast if the client is used to send a request to the wrong host. But it would be counter to the way net/http clients usually work, so perhaps too non-intuitive for Go developers?

[jchadwick-buf]

Overall looks like a reasonable starting point to me. I left some comments but they're just as much discussion points as anything else.

[jchadwick-buf]

Thinking out loud: should we use a context for the resolver requests? Seems like we could get two birds with one stone on that one, since we need one to pass to the resolver anyways, plus it gives us deadline support all the way through.

[jhump]

Yes, we'll definitely want a context for the resolver requests. I didn't bother weaving any of that through yet since it's not clear where it would come from anyway (probably a client option when the client is created?)

[jchadwick-buf]

Oh, I didn't explain this very well. I mean to say, we have a channel here to watch for resolver completion, but contexts have a Done channel already. So if we create a context with deadline (probably off of something passed in through client options, yeah) we could store that context to watch for when the resolver is finished. It can also be used to keep an err value for the resolver, in case we wanted to bubble up errors in a deferred fashion after a resolver failure.

[jchadwick-buf]

I have been working on exploring other data structures we could potentially use for better performance with subsetting. I'm thinking that an efficient HAMT implementation (that is, not using slice headers everywhere) would work very well for storing the hosts. For small numbers of hosts, it wouldn't really give us too much benefit, but I don't think it's unreasonable that someone may want to have >100 hosts, at which point persistent immutable lock-free structures seem like something we really want.

The way I envision this working with a subsetter/resolver interface, we just need a way to get a diff at the end of the process so we can apply it to the HAMT, then one atomic pointer swap should do the trick once we've constructed the new HAMT root.

(There are some existing HAMT implementations in Go, but I'm not all that impressed by the efficiency of them, especially considering it's a relatively simple data structure...)

[jhump]

I'd be a little wary of the HAMT approach. A map is mainly useful for lookup by address, which is really only needed when reconciling the addresses with new results from the resolver -- which does not happen often. Also, for many picking algorithms, we'll need random access (like for random or power-of-two, but also really for round robin since that's most efficient to just store and bump an index), and an HAMT doesn't provide good random access. (With a tweak to the structure, it could be done in ~ O(log n) time.) So I think the slice is likely the best data structure to expose to pickers, paired with something else as an index (for lookup by address, internally).

[akshayjshah]

Generally, I like this API! I especially like that it provides an http.Client, which I think most Go developers expect.

I do agree that we're likely to end up with lots of hostname-specific options, which may end up being difficult to grok. I think we may be able to minimize that by introducing a second layer of options:

type HostOption interface { ... }

func WithHostOptions(hostPort string, options ...HostOption) ClientOption { ... }

We'd probably want to pair this with a default behavior of failing requests to unconfigured domains and an option to explicitly set a fallback http.RoundTripper.

Either way, we can cross this bridge when we get to it. This API looks like a good starting place to me!

[akshayjshah]

connect-go has what I think is a useful setup for exhauststruct:

linters-settings:
  exhaustruct:
    include:
      # No zero values for param structs.
      - 'github\.com/bufbuild/connect-go\..*[pP]arams'

It's just helping use someFunctionParams structs as a way to clean up functions with too many arguments without allowing implicit zero value parameters. Might be useful here, might not - up to you.

[akshayjshah]

httplb?

Also...did someone explicitly ask us to make the repository name not match the package name? We do this often, but I don't see it much in the general OSS community. I also don't imagine us having name conflicts if we end up doing this in other languages.

[jhump]

We hadn't really put any thought into the repo name yet. If we used httplb as the package name, maybe we can get away with just renaming the repo to httplb-go? Or perhaps it should be gohttplb, so the package is short and meaningful and the repo name is also reasonably self-describing?

I think many other projects don't have an issue with repo name != package name because they may have only Go repos (so no need to add "go" to the name of the repo, to distinguish it from other non-Go code) or they have larger "mono"-repos, where the repo name might have "go" in it, but all of the actual packages are sub-directories in the repo. (For something like this, making a sub-directory would be a little annoying as its just an extra component of import path for no good reason...)

[akshayjshah]

Yeah. I don't mind too much either way - let's just keep this as-is and save all the name bikeshedding for the last mile of the project :)

[jchadwick-buf]

I'm cool with go-httplb (edit: or httplb or httplb-go), this name was literally just something I dumped out so that I didn't spend too much time trying to name the folder when I was first experimenting. :)

[akshayjshah]

With the designs we're thinking about, I suspect that this is just the first of many goroutines that we're going to create. I expect that some of them will (roughly) live for the life of the top-level client. I'd really, really prefer that we provide a way for users to close the client and block until all goroutines have exited. I'm guessing that'll require a top-level Client struct that this package owns:

type Client {
  *http.Client
  
  ...some internal stuff...
  }
  
func (c *Client) Close() error { ... }
// or, if it makes more sense...
func (c *Client) Shutdown(ctx context.Context) error { ... }

I'm guessing that we'd also need a for-now-internal transport type that wraps http.RoundTripper in a similar way.

[jhump]

What I had started on was simply a way to pass a "root context" when creating the client, and cancelling the root context would free resources and shutdown goroutines. But, admittedly, that doesn't provide a way to block until things are closed.

Though, FWIW, the http.Transport also starts goroutines but provides no way to actually clean them up or shutdown connections must less a way to block for all resources to be released. (It only offers a CloseIdleConnections() method, which provides no feedback on any connections that were not closed because they were ostensibly still in use.)

Given the existing defacto client and transport options don't offer such an API, do you think it's really necessary for this package to provide that? We don't really have a perfect way to implement that sort of thing given that we are using *http.Transport at the leaf-level, and have no way to really clean them up.

[akshayjshah]

I still think it's valuable, since we're never quite sure what new interface upcasts we're going to get from the stdlib (case in point: I didn't think stream.Send would ever have a use for contexts, but now there's http.ResponseController and I regret my choices).

I think it's still valuable to commit to closing all the goroutines we start. I suspect we'll also often own the dial function, so we can wire a WaitGroup into conn.Close too. Agree that it won't be perfect, but a best effort seems like it might end up being pretty good.

For context, I think a bunch of this is scar tissue from writing and slowly rolling out https://github.com/uber-go/goleak - so many libraries randomly leaked goroutines that it was really difficult to identify new leaks.

[jhump]

LOL, yeah, that's fair. In my personal projects, I have implemented tests for this, too. It's just harder when using net/http, since it creates goroutines and then provides no good controls for cleaning them up ☹️

I can actually go ahead and add that to the existing smoke test (and keep it as a check on many more tests to be added).

BTW, I don't think we'd need a wrapper Client type. We could instead provide a function that accepts *http.Client and returns an error if the client wasn't created with this package. (Easy to check via type assertion on the transport, like this.)

[jhump]

@akshayjshah, re: the HostOption stuff, I do like that! This intersects interestingly with the topic of "pre-warming". Do we treat all explicitly configured hosts as needing to remain warm (and need to be pre-warmed)? I imagine the answer to be "yes".

I started the implementation so that each transportPool is per scheme plus host:port, but perhaps it is better for most configuration to be just per host:port, and then inside the transportPool we could group leaf transports by scheme. (The scheme is important since it could mean we build a different/incompatible transport, like for "h2c".) Thoughts?

[akshayjshah]

Do we treat all explicitly configured hosts as needing to remain warm (and need to be pre-warmed)?

That makes sense to me, yeah. Why preconfigure settings if you don't expect to use them?

I started the implementation so that each transportPool is per scheme plus host:port, but perhaps it is better for most configuration to be just per host:port, and then inside the transportPool we could group leaf transports by scheme.

Eh, this is a good question. Fewer layers sounds better to me, especially since some options are likely to be scheme-specific. Maybe we use "backend" instead of "host" and do WithBackend(scheme, hostPort string, opts ...BackendOption) ClientOption?

Edit: Actually, maybe we end up with WithHTTPBackend(...), WithHTTP2Backend(...), and WithH2CBackend(...)? IDK, let's see what ends up feeling good. I think it'll depend on how many transport-specific options we end up with, and whether we want to support slightly more exotic transports like Unix domain sockets.

[jhump]

@akshayjshah, yeah, was just about to post something similar. I realized that we really do need the scheme in there since so much relies on it (particularly transport creation, which is likely a prerequisite for most health checkers, which is in turn needed for warming up the client).

Edit: Actually, maybe we end up with WithHTTPBackend(...), WithHTTP2Backend(...), and WithH2CBackend(...)? IDK, let's see what ends up feeling good.

I prefer to omit this for now. Mainly because I see it being quite reasonable (in terms of effort/maintenance) to offer the ability to use custom schemes, like "http3", and don't want the API to preclude that. We can certainly provide scheme constants.

[akshayjshah]

That makes sense too!