New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
all: consider a way to automatically Close clients #4498
Comments
Sounds like a great idea. We are already half way there - we have a wrapper Client type with the transport-specific The integration tests in the client generator do make use of a copy of One question: if we roll this out, is there a baked-in break glass API for users to disable our use of finalizers? Or do we need to consider adding something to toggle this off? |
(I'm going to repeat here what I said on the thread with Russ.) I'm really not a fan of implicitly closing things using finalizers as is done for Finalizers work better for verifying invariants than for restoring them. For example, here you could have the finalizer Close the client when run in non-race mode (to avoid OOMs in production), but terminate the process if the client is unclosed in race mode (to detect leaks during testing): func NewClient() *Client {
c := &Client{newInternalClient()}
if raceMode {
c.addAllocationStackTrace()
}
runtime.SetFinalizer(c, (*Client).finalize)
return c
}
func (c *Client) Close() error {
runtime.SetFinalizer(c, nil)
return …
}
func (c *Client) finalize() {
if raceMode {
c.dumpAllocationStackTrace()
panic(fmt.Sprintf("%T became unreachable without a call to Close", c))
} else {
⋯ // Maybe log a warning or increment a metric here?
c.i.Close()
}
} That provides the same benefits in production as just invoking That would also avoid the need for (fragile) use of |
To highlight one specific ill effect from closing in finalizers: finalizers are run by the garbage collector, and the garbage collector paces its own progress (and thus the rate of finalization) in proportion to memory usage (controlled by the However, the resources consumed by a client are not necessarily only Go memory: clients may also need network sockets, file descriptors, or other similar handles to kernel-managed resources. It is possible for a Go program to run out of those resources well before it runs out of memory proper, in which case the program will start to fail. This failure mode will typically happen under load — so users may be less likely to test for it adequately — and is not usually reflected in benchmarks (which typically measure memory and CPU resources, not network sockets and file descriptors). So I believe that a solution that encourages users to rely on finalizers to close clients will lead to user programs that are more likely to fail under moderate to high loads. |
IMHO a specific linter is a very good option without touching original code, like https://github.com/timakin/bodyclose does for |
I think we could implement this at the pool level: the pool could drain connections if it detects it has been left unused. Every time a client makes a gRPC call, https://github.com/googleapis/google-api-go-client/blob/v0.52.0/transport/grpc/pool.go#L41 |
An IMHO straight forward way is to introduce a package level function, e.g.
A client uses it as It‘s a bit more complicated, if |
pubsublite is now supporting this finalizer behavior: #7109 |
Based on some recommendations from the Go language team I don't think we will move forward with auto-closing via finalizers. I think this is a docs problem which we have done audits in the past to make sure our samples all show closing clients. More recently I have also noticed auto-completes (AI based) recommend closing clients as well. If a different strategy comes up in the future on how to handle closing clients we can consider it but we will not be moving forward with a general purpose finalizer approach. |
Users have to remember to
Close
clients when they're done using them. Otherwise, you can end up with a memory leak. Is there a way we can automaticallyClose
clients?@rsc suggested using finalizers. We need to take care to use them correctly and not cause more problems (either with leaks or doing "work" that shouldn't be done automatically).
cc @bcmills @codyoss @noahdietz @rsc
The text was updated successfully, but these errors were encountered: