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!

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.

grpc/src/client/channel.rs

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.

grpc/src/client/channel.rs

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

dfawley · 2026-01-12T22:42:29Z

grpc/src/client/channel.rs

+    /// Returns the current state of the channel. If there is no underlying active channel,
+    /// returns Idle. If `connect` is true, will create a new active channel.


This leaks implementation details into the documentation. Users shouldn't know about our active channel at all. How about something like:

Suggested change

/// Returns the current state of the channel. If there is no underlying active channel,

/// returns Idle. If `connect` is true, will create a new active channel.

/// Returns the current state of the channel. If `connect` is true, will begin connecting.

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 2 times, most recently 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 force-pushed the refactor/PersistentChannel branch 2 times, most recently from a1cca14 to 861d86f Compare January 8, 2026 23:04

dfawley reviewed Jan 8, 2026

View reviewed changes

nathanielford added 7 commits January 12, 2026 18:04

Separate channel creation and state probe, and error handling thereof

b7ba18c

Remove unused import

e59bb57

Switch to options over Result to address PR concerns.

2c0cf4b

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

199fc15

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

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

2c5b5a3

Remove one-liner get lock. Use clone()

d5e870a

Fix comment.

ed26b0e

nathanielford force-pushed the refactor/PersistentChannel branch from 1130c0a to ed26b0e Compare January 12, 2026 18:04

Fix comment

2f6b056

nathanielford enabled auto-merge (squash) January 12, 2026 22:11

dfawley approved these changes Jan 12, 2026

View reviewed changes

nathanielford merged commit 7e98e1f into hyperium:master Jan 12, 2026
20 checks passed

		/// Returns the current state of the channel. If there is no underlying active channel,
		/// returns Idle. If `connect` is true, will create a new active channel.

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

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants