Separate channel creation and state probe, and error handling thereof. #2465

nathanielford · 2025-12-15T22:04:09Z

Motivation

Currently the Channel object is calling into the PersistentChannel internals to do things like provision an active channel and check state. The division of where work and error handling is managed becomes muddied in this situation, which complicates handling errors in ways that aren't unwrap.

This was also motivated by work to implement the always-failing PersistentChannel PR, but it seemed like it could be broken off into it's own PR.

Solution

Moving the actual mechanics into the PersistentChannel object, which owns channel config, and allows Channel to be a lightweight handles for users to interact with. Specifically state probing and channel creation are now in PersistentChannel.

PersistentChannel calls are wrapped in Result objects that are translated at the Channel level to the appropriate public-facing API objects. get_or_create_active_channel() just becomes get_active_channel() and passes the 'create' flag, which controls whether a new channel is created if it does not already exist.

The external semantics should stay the same as they have been.

This does not yet address the error that can surface in call(), because it's not yet clear what the error reporting story is.

dfawley · 2025-12-17T23:59:56Z

grpc/src/client/channel.rs

-        if let Some(s) = ac.connectivity_state.cur() {
-            return s;
+        let state = self.inner.state(connect);
+        match state {


Wouldn't we want the two state() methods to have the same return value so this could simply delegate?

dfawley · 2025-12-18T00:05:23Z

grpc/src/client/channel.rs

+        let mut s = self
+            .active_channel
+            .lock()
+            .map_err(|_| "Could not get channel lock.".to_string())?;


I think this can stay as unwrap(). IIUC locks can only fail in pretty spectacular ways (someone holding the lock panicked). In that case we might as well panic too.

I went back and forth on this, but ultimately decided to leave it as unwrap() on the theory that we really do want lock poisoning to cause an all-stop, and fix whatever causes the poisoning rather than allow apps to recover from it. I'm only vaguely uncomfortable with leaving the unwrap in there.

dfawley · 2025-12-18T00:09:47Z

grpc/src/client/channel.rs

+        s.as_ref()
+            .cloned()
+            .ok_or_else(|| "Could not clone channel".into())


By here you know s is Some, so you can do s.unwrap().clone() I think.

Or if you do a if let Some(ac) = s else {} instead of if s.is_none() then it will already be unwrapped.

dfawley · 2025-12-18T00:10:15Z

grpc/src/client/channel.rs

+        &self,
+        connect: bool,
+    ) -> Result<Arc<ActiveChannel>, Box<dyn std::error::Error + Sync + Send>> {
+        let mut s = self


Maybe name this ac or active_channel? I'm not sure what s represents.

dfawley · 2025-12-18T00:13:33Z

grpc/src/client/channel.rs

+                    self.runtime.clone(),
+                ));
+            } else {
+                return Err("No active channel.".into());


This function can probably return just an Option?

Also maybe we shouldn't combine these two functionalities into one function. They're a little disjoint.

Anyone asking for the active channel itself will always want connect=true.

Only when requesting the current state, exiting idle mode is optional.

Without needing to be clear that the error is a lock poisoning, the option makes the most sense, so I changed it to this.

I also removed the bool flag after going back and forth with various options. After being satisfied that I could arrange the deck chairs any which way settled on the current version. LMK what you think!

…ake the decision to avoid this only in state().

dfawley

I'm definitely +100 on the motivation behind this change -- the ActiveChannel should be the thing fiddling with its own state, not the PersistentChannel. A few minor nits, but otherwise LGTM, thanks.

dfawley · 2026-01-08T23:17:30Z

grpc/src/client/channel.rs

-    // Channels begin idle so new is a simple constructor.  Required parameters
-    // are not in ChannelOptions.
+    // Channels begin idle so new does not automatically connect.
+    // ChannelOptions are only non-required parameters.


Nit:

Suggested change

// ChannelOptions are only non-required parameters.

// ChannelOptions contains only optional parameters.

dfawley · 2026-01-08T23:27:58Z

grpc/src/client/channel.rs

+        let active_channel = if connect {
+            self.get_active_channel()
+        } else {
+            match self.locked_active_channel().as_ref().cloned() {


as_ref().cloned() -> clone() ? (And below)

dfawley · 2026-01-08T23:29:21Z

grpc/src/client/channel.rs

+    // Internal use only to get the locked active channel. If this panics it means the lock is
+    // poisoned and we should also panic.
+    fn locked_active_channel(&self) -> std::sync::MutexGuard<'_, Option<Arc<ActiveChannel>>> {
+        self.active_channel.lock().unwrap()


I am not sure having a method for this one liner is helpful, and in fact makes the call sites more opaque which is probably a bad thing.

The only reason I thought it was worth it was to try and put the unsafe unwrap in one place. But I'll revert it, since I think it's probably not that big a deal.

dfawley · 2026-01-08T23:32:43Z

grpc/src/client/channel.rs

+    /// Returns the current state of the channel. Any errors translate into a
+    /// TransientFailure state.


I'm not sure what the "translate" comment is about. Does that mean there could be an error while simply retrieving the state? I don't think this is possible or appropriate...this method should just tell me the current state of the channel, period.

nathanielford force-pushed the refactor/PersistentChannel branch from 51ee8a7 to ab8eb2d Compare December 15, 2025 22:46

nathanielford marked this pull request as ready for review December 16, 2025 00:17

nathanielford force-pushed the refactor/PersistentChannel branch from eca5d2b to ab8eb2d Compare December 16, 2025 19:54

nathanielford added 2 commits December 16, 2025 11:55

Separate channel creation and state probe, and error handling thereof

4c070f1

Remove unused import

aa24f63

nathanielford force-pushed the refactor/PersistentChannel branch from ab8eb2d to aa24f63 Compare December 16, 2025 19:55

nathanielford requested a review from dfawley December 16, 2025 23:49

nathanielford self-assigned this Dec 16, 2025

dfawley reviewed Dec 18, 2025

View reviewed changes

nathanielford added 2 commits January 8, 2026 18:34

Switch to options over Result to address PR concerns.

cc81508

Remove connect flag from get_active_channel. Always try to connect. M…

3ae848e

…ake the decision to avoid this only in state().

nathanielford force-pushed the refactor/PersistentChannel branch from 603cd75 to a1cca14 Compare January 8, 2026 23:00

Remove option from get_active_channel, since it can never be None.

861d86f

nathanielford force-pushed the refactor/PersistentChannel branch from a1cca14 to 861d86f Compare January 8, 2026 23:04

Merge branch 'master' into refactor/PersistentChannel

1130c0a

dfawley reviewed Jan 8, 2026

View reviewed changes

	// ChannelOptions are only non-required parameters.
	// ChannelOptions contains only optional parameters.

		/// Returns the current state of the channel. Any errors translate into a
		/// TransientFailure state.

Separate channel creation and state probe, and error handling thereof. #2465

Are you sure you want to change the base?

Separate channel creation and state probe, and error handling thereof. #2465

Conversation

nathanielford commented Dec 15, 2025

Motivation

Solution

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dfawley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants