Is it better not to enable WithIdleTimeout by default?

[LugiaChang]

I previously used grpc-go to implement grpc proxy functionality and customized resolver and balancer, with some goroutines constantly listening for address changes. I also saved the resolver instances for external http proxy usage. However, after the feature in #6263 was merged, I found that the resolver and balancer would be recycled after a certain period of time, and my resolver/balancer goroutines were not properly recycled, causing overflow; moreover, the http proxy became ineffective until a new grpc rpc reactivated it. I believe this feature changes the original usage pattern and it would be better to have it disabled by default?

[easwars]

Channel idleness is a useful feature in various deployments. It is enabled by default in other gRPC languages and we would like to keep in enabled by default in grpc-go as well.

If channel idleness is causing problems for you, we would like to hear more about it and see if there are any issues with our implementation that needs fixing. Else, you can always disable channel idleness in your application by passing an idle timeout of zero to this dial option.

[dannyskoog]

We also started experiencing issues some weeks ago (with goroutine count growing over time for certain k8s pods belonging to low-traffic services of ours) when updating to v1.59.0 (where IdleMode was enabled by default). We have reverted to v1.58.3 for the time being. Additionally we suspect that 5d7453e will solve the issue we faced with v1.59.0 once it's released.

I just wanted to share this in case it helps anyone that's facing similar issues.

[easwars]

@dannyskoog : Thank you for reporting your problem. Did you manage to get a goroutine dump before reverting back to 1.58? Or would you be able to provide us with any other information that will help us figure out if there are issues in implementation. Thanks.

[dannyskoog]

@easwars We did use pprof during the investigation IIRC. But I can't promise that I have something to provide. I'll check and get back to you at least 👍

[sanposhiho]

We also encountered issues due to this change. I know we can opt-out this default value, and the issue may (or may not) be our fault in the implementation. But, the problem here is that regardless of whether it causes any issue or not, changing default values should be treated as a breaking change, not a new feature.

grpc: channel idleness enabled by default with an idle_timeout of 30m (#6585)
https://github.com/grpc/grpc-go/releases/tag/v1.59.0

[dfawley]

But, the problem here is that regardless of whether it causes any issue or not, changing default values should be treated as a breaking change, not a new feature.

There shouldn't be any visible effects of this feature except a little additional latency in new RPCs after the channel is idle for 30 minutes, so it would be a stretch to call this a breaking change. Meanwhile, it provides a benefit of reducing connections on servers from idle clients.

Even though we try our best, bugs are inevitable in all components of the system (and in this case there was one that is fixed in our upcoming release today/tomorrow). This feature brings us into better alignment with all the other gRPC languages, so we will not be disabling it by default.

[sanposhiho]

I wouldn't object to the decision itself anymore though (I don't have any right in this project anyway), but I have to say... changing a default value in general is all breaking change. Even if changing a default value/behaviour should not impact badly to users, we, open-source maintainers, should treat them breaking changes basically.

[dfawley]

We typically don't change defaults if we believe there could be any adverse affects, but in order to improve the project, we do need to be practical and allow some changes that are better for the majority of our users when no visible downsides are anticipated.

Note that if you are using the experimental name resolver / load balancer packages, then they are subject to changes in behavior & API as we work to adapt them for more use cases (most notably IPv4/IPv6 dual-stack; see #6472 / grpc/proposal#356).