core: Fix NPE race during hedging #10007
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The problem was one hedge was committed before another had drained start(). This was not testable because HedgingRunnable checks whether scheduledHedgingRef is cancelled, which is racy, but there's no way to deterministically trigger either race. The same problem couldn't be triggered with retries because only one attempt will be draining at a time. Retries with cancellation also couldn't trigger it, for the surprising reason that the noop stream used in cancel() wasn't considered drained. This commit marks the noop stream as drained with cancel(), which allows memory to be garbage collected sooner and exposes the race for tests. That then showed the stream as hanging, because inFlightSubStreams wasn't being decremented. Fixes grpc#9185
|
Friendly ping |
temawi
approved these changes
Apr 11, 2023
YifeiZhuang
approved these changes
Apr 11, 2023
ejona86
added
the
TODO:backport
This was referenced Apr 13, 2023
ejona86
removed
the
TODO:backport
zeebe-bors bot
added a commit
to zeebe-io/zeebe-cluster-testbench
that referenced
this pull request
May 17, 2023
828: fix(deps): update dependency io.grpc:grpc-api to v1.55.1 r=korthout a=renovate[bot] [](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [io.grpc:grpc-api](https://togithub.com/grpc/grpc-java) | `1.54.0` -> `1.55.1` | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>grpc/grpc-java</summary> ### [`v1.55.1`](https://togithub.com/grpc/grpc-java/releases/tag/v1.55.1) [Compare Source](https://togithub.com/grpc/grpc-java/compare/v1.54.1...v1.55.1) The 1.55.0 release failed. There were no artifacts published for it. ##### API Changes - services: Rename `MetricRecorder.setQps`/`clearQps` to `setQpsMetric`/`clearQpsMetric` ([#​10031](https://togithub.com/grpc/grpc-java/issues/10031)) ##### Behavior Changes - gcp-observability: Remove monitored resource detection for logging ([grpc/grpc-java#10020). The cloud libraries will fill in these details instead - protoc-gen-grpc-java: binaries for Linux ARM and PPC are now built using Ubuntu 18.04. They will no longer work on Ubuntu 16.04 and Debian 9 ##### New Features - api: Stabilize the frequently used compression APIs ([#​9942](https://togithub.com/grpc/grpc-java/issues/9942)): `CallOptions.withCompression`, `CallOptions.getCompressor`, `AbstractStub.withCompression`, `ServerCall.setCompression`, `ServerCall.setMessageCompression` - api: Stabilize `Detachable` and `HasByteBuffer` - gcp-observability: Stabilize `GcpObservability` ([grpc/grpc-java#10024). The GcpObservability API provides a simple way to export logging, tracing, and metrics to Google Cloud Operations. See [the Google Cloud blog post](https://cloud.google.com/blog/products/networking/introducing-grpc-observability-for-microservices). - census: Add new tracer annotation to indicate the time when name resolution completed for those RPCs that experienced name resolution delay, or the time when picking subchannel completed for those RPCs that experienced picking subchannel delay. ([#​10014](https://togithub.com/grpc/grpc-java/issues/10014), [#​10044](https://togithub.com/grpc/grpc-java/issues/10044)) - protoc-gen-grpc-java: binary for s390x is now published ([#​9455](https://togithub.com/grpc/grpc-java/issues/9455)). The glibc version used is available in Ubuntu 20.04, Debian 11, and CentOS 9 and later - authz: Added `FileWatcherAuthorizationServerInterceptor` ([#​9775](https://togithub.com/grpc/grpc-java/issues/9775)) - services: Added `OrcaMetricReportingServerInterceptor.create(MetricRecorder)` which adds common metrics per-RPC ([#​9902](https://togithub.com/grpc/grpc-java/issues/9902)) - android: Add `UdsChannelBuilder` for using LocalSocket an Android ([#​8418](https://togithub.com/grpc/grpc-java/issues/8418)) - alts: Observe the `GRPC_ALTS_MAX_CONCURRENT_HANDSHAKES` environment variable user to adjust the max number of concurrent ALTS handshakes ([#​10016](https://togithub.com/grpc/grpc-java/issues/10016)) - binder: Expose client identity via `PeerUid` and `PeerUids` ([#​9952](https://togithub.com/grpc/grpc-java/issues/9952)) - binder: Add `BindServiceFlags.setAllowActivityStarts()` for `BIND_ALLOW_ACTIVITY_STARTS` added in Android U ([#​10008](https://togithub.com/grpc/grpc-java/issues/10008)) ##### Bug Fixes - core: Fix NPE race during hedging ([grpc/grpc-java#10007), fixing a Netty buffer memory leak for cancelled RPCs - core: Allow transparent retries after a retry attempt and the configured max retries was 1 ([#​10066](https://togithub.com/grpc/grpc-java/issues/10066)) - okhttp: properly implement `OkHttpServerBuilder.maxConnectionAgeGrace()` ([#​9968](https://togithub.com/grpc/grpc-java/issues/9968)) - xds: Enable federation support. See [gRFC A47](https://togithub.com/grpc/proposal/blob/master/A47-xds-federation.md) - xds: Enable Weighted Round Robin LB policy support. See [gRFC A58](https://togithub.com/grpc/proposal/blob/master/A58-client-side-weighted-round-robin-lb-policy.md) - xds: Avoid ClassCastException if the control plane changes the top-level policy ([#​10091](https://togithub.com/grpc/grpc-java/issues/10091)). This is expected to be unlikely, but is possible - xds: Fix `java.util.NoSuchElementException: SecurityProtocolNegotiators$ClientSdsHandler#​0` ([#​10118](https://togithub.com/grpc/grpc-java/issues/10118)). This error did not cause any problems, other than unnecessary logging - xds: Avoid using the default locale for case insensitive path matching ([#​10148](https://togithub.com/grpc/grpc-java/issues/10148)) - googleapis: Enable ignore_resource_deletion for `google-c2p:` resolver’s default xds bootstrap ([#​10121](https://togithub.com/grpc/grpc-java/issues/10121)) - rls: Refresh name resolution on rejected addresses ([#​10032](https://togithub.com/grpc/grpc-java/issues/10032)) ##### New Examples - Keepalive ([#​9956](https://togithub.com/grpc/grpc-java/issues/9956)) - Cancellation ([#​9962](https://togithub.com/grpc/grpc-java/issues/9962)) - Deadline ([#​9958](https://togithub.com/grpc/grpc-java/issues/9958)) - Using waitForReady ([#​9960](https://togithub.com/grpc/grpc-java/issues/9960)) - Client and Server sharing ([#​9969](https://togithub.com/grpc/grpc-java/issues/9969)) - Reflection ([#​9955](https://togithub.com/grpc/grpc-java/issues/9955)) - Doing debug ([#​9957](https://togithub.com/grpc/grpc-java/issues/9957)) - Health service ([#​9991](https://togithub.com/grpc/grpc-java/issues/9991)) - Error details ([#​9997](https://togithub.com/grpc/grpc-java/issues/9997)) - Custom load balancing ([#​9951](https://togithub.com/grpc/grpc-java/issues/9951)) - gRPC-level reverse proxy ([#​10059](https://togithub.com/grpc/grpc-java/issues/10059)) ##### Dependencies - protobuf-java and protobuf-java-util upgraded to 3.22.3 ([#​10045](https://togithub.com/grpc/grpc-java/issues/10045)) ##### Acknowledgements - [`@​carl-mastrangelo](https://togithub.com/carl-mastrangelo)` - [`@​haubenr](https://togithub.com/haubenr)` - [`@​jpd236](https://togithub.com/jpd236)` - [`@​kenk42292](https://togithub.com/kenk42292)` ### [`v1.54.1`](https://togithub.com/grpc/grpc-java/releases/tag/v1.54.1) [Compare Source](https://togithub.com/grpc/grpc-java/compare/v1.54.0...v1.54.1) ##### Bug Fixes - core: Fix NPE race during hedging ([grpc/grpc-java#10046), fixing a Netty buffer memory leak for cancelled RPCs ##### Behavior Changes - gcp-observability: Remove monitored resource detection for logging ([grpc/grpc-java#10026). The cloud libraries will fill in these details instead ##### API stabilizations - Stabilize GcpObservability ([grpc/grpc-java#10027) - The GcpObservability API provides users with a simple way to export logging, tracing, and metrics to Google Cloud Operations. For more information, please see [this blog post](https://cloud.google.com/blog/products/networking/introducing-grpc-observability-for-microservices). </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/zeebe-io/zeebe-cluster-testbench). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS43NC4wIiwidXBkYXRlZEluVmVyIjoiMzUuODkuMSIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
](https://renovatebot.com){kind=link}
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The problem was one hedge was committed before another had drained start(). This was not testable because HedgingRunnable checks whether scheduledHedgingRef is cancelled, which is racy, but there's no way to deterministically trigger either race.
The same problem couldn't be triggered with retries because only one attempt will be draining at a time. Retries with cancellation also couldn't trigger it, for the surprising reason that the noop stream used in cancel() wasn't considered drained.
This commit marks the noop stream as drained with cancel(), which allows memory to be garbage collected sooner and exposes the race for tests. That then showed the stream as hanging, because inFlightSubStreams wasn't being decremented.
Fixes #9185