google-cloud-rust icon indicating copy to clipboard operation
google-cloud-rust copied to clipboard

Decision on Response Extensions and UnwindSafe

Open westarle opened this issue 3 months ago • 2 comments

Status: Draft

Date: 2025-10-06

Context:

To support span enrichment and allow passing data from transport layers, we need to incorporate http::Extensions into google_cloud_gax::Response::Parts.

A key constraint is that google_cloud_gax::Response is currently UnwindSafe. However, http::Extensions is not UnwindSafe. Directly including http::Extensions in Parts would remove the UnwindSafe guarantee from google_cloud_gax::Response. This is considered a breaking change by tools like cargo-semver-checks (see obi1kenobi/cargo-semver-checks#function_abi_no_longer_unwind), as it can affect users relying on catch_unwind.

Decision:

To maintain backward compatibility and the UnwindSafe guarantee, we will wrap http::Extensions within an Arc<Mutex<>> in the Parts struct:

// In src/gax/src/response.rs
pub struct Parts {
    // ... other fields ...
    pub extensions: Arc<Mutex<http::Extensions>>,
}

The Response<T> type will provide extensions(&self) and extensions_mut(&mut self) methods that return MutexGuard<'_, http::Extensions>, panicking if the mutex is poisoned.

Consequences:

  • Pros: Maintains UnwindSafe without a major version bump. The API on Response<T> for accessing extensions (extensions, extensions_mut) mimics the hyperium/http crate's style.
  • Cons: Introduces runtime overhead due to locking. Accessing extensions requires handling the MutexGuard's lifetime, and calls can panic if the mutex is poisoned.

Alternatives Considered:

  1. Use http::Extensions directly: Rejected because it's a breaking change for UnwindSafe in the current major version.
  2. Silence cargo-semver-checks: Rejected as it hides a real potential breakage for users of catch_unwind.
  3. Bump to 2.0.0: Deferred to avoid imposing an immediate major version bump on users. This can be revisited in a future major release, at which point the Arc<Mutex<>> wrapper could be removed.

westarle avatar Oct 06 '25 11:10 westarle

Cons: Introduces runtime overhead due to locking. Accessing extensions requires handling the MutexGuard's lifetime, and calls can panic if the mutex is poisoned.

Also introduces runtime overhead because creating a Response<T> is more expensive

The Response<T> type will provide extensions(&self) and extensions_mut(&mut self) methods that return MutexGuard<'_, http::Extensions>, panicking if the mutex is poisoned.

What happens if we ever bump to 2.0? Do we need to change this API too? Is that more breaking than we want? Should we hide those accessors in pub(crate) or #[doc(hidden)] or in an internal namespace?

Alternatives Considered:

Can we skip the http::Extensions and add the o12y annotations directly? Same questions about making any accessors to them pub(crate) or #[doc(hidden)] or in an internal namespace?

coryan avatar Oct 06 '25 13:10 coryan

FYI, in google-cloud-cpp we record these decisions in adr documents created via a PR. It is easier to make suggestions and track comments in PRs.

No worries, writing things down and having the discussion is more important than the specific format.

coryan avatar Oct 06 '25 13:10 coryan