Skip to content

fix(util): make option_layer unify branch errors to BoxError#876

Open
ameyypawar wants to merge 5 commits into
tower-rs:masterfrom
ameyypawar:fix/option-layer-boxerror
Open

fix(util): make option_layer unify branch errors to BoxError#876
ameyypawar wants to merge 5 commits into
tower-rs:masterfrom
ameyypawar:fix/option-layer-boxerror

Conversation

@ameyypawar

Copy link
Copy Markdown
Contributor

Summary

Fixes #665.

option_layer returns Either<L, Identity>. The service-level Either<A, B> requires both branches to share an error type (B: Service<Request, Response = A::Response, Error = A::Error>). So when the optional layer changes the error type — e.g. TimeoutLayer, whose service errors are BoxError — over a service with a different error type, the result does not implement Service at all:

let svc = ServiceBuilder::new()
    .option_layer(Some(TimeoutLayer::new(Duration::from_secs(1))))
    .service_fn(|()| async { Ok::<_, io::Error>(()) }); // BoxError vs io::Error -> not a Service

The existing doc test didn't catch this because it never asserted that the result is a Service.

Why the fix has to box at the service level

A generic Layer<S> impl can't name S's error type — Layer has no Request parameter, so <S as Service<Request>>::Error isn't in scope when the layer is applied. The error types are only known once a request type is fixed, i.e. in the produced service's own Service impl. So the unification can't be done by composing MapErrLayer at option_layer time; it must live in a dedicated service.

The fix

Add OptionLayer<L> (the layer now returned by option_layer). Its produced OptionService<A, B> is like Either, but its Service impl sets Error = BoxError with bounds A::Error: Into<BoxError>, B::Error: Into<BoxError>, and B::Response = A::Response — so the layered and unlayered branches need not share an error type. ServiceBuilder::option_layer's return type is updated to match.

// before:  pub fn option_layer<L>(layer: Option<L>) -> Either<L, Identity>
// after:   pub fn option_layer<L>(layer: Option<L>) -> OptionLayer<L>

⚠️ Breaking change — and this is a proposal

This is semver-breaking and adds public API, so I'm raising it as a proposal rather than assuming the shape:

  • Changed return types (breaking): util::option_layer (Either<L, Identity>OptionLayer<L>) and ServiceBuilder::option_layer (Stack<Either<T, Identity>, L>Stack<OptionLayer<T>, L>).
  • New public items: util::OptionLayer, util::OptionService, and util::future::OptionResponseFuture.
  • Names/placement are open to your preference (OptionService's Some/None variants, the OptionResponseFuture alias, etc.).
  • An alternative discussed in Either rework broke option_layer #665 was impl Layer<S> for Option<L>; that has the same error-unification need, so the OptionService boxing machinery would still be required underneath. Happy to reshape toward whichever direction you prefer.

Tests / verification

  • Adds a regression test (tests/util/option_layer.rs) that builds option_layer over a layer which changes the error type and asserts the result is a usable Service. It fails to compile before this change (the two Either branches have different error types) and passes after.
  • Locally verified: cargo fmt --check, cargo check --workspace --all-features --all-targets, the new test, and the option_layer/ServiceBuilder::option_layer doc tests all pass; clippy and the all-features build are warning-clean. The change uses only match / Poll::map_err / Into::into, so it's within the current MSRV.

Introduce `OptionLayer`, the layer produced by `option_layer`. Unlike using
`Either` directly, its service unifies the layered and unlayered branches'
error types to `BoxError`, so an optional layer that changes the error type
(e.g. `TimeoutLayer`) no longer breaks the resulting `Service` (tower-rs#665).

This commit only adds the new module; `option_layer` is rewired to it in a
follow-up commit.
…rror)

`option_layer` returned `Either<L, Identity>`, whose `Service` impl requires
both branches to share an error type. When the optional layer changes the
error type (e.g. `TimeoutLayer`), the resulting service did not implement
`Service` at all (tower-rs#665).

Return the new `OptionLayer` instead, which unifies the layered and unlayered
branches' error types to `BoxError`. Re-export `OptionLayer`/`OptionService`
and the response future from `util`.
Reflect `option_layer` now returning `OptionLayer<T>` instead of
`Either<T, Identity>`.
…-rs#665)

Build `option_layer` over a layer that changes the error type and assert the
result is a usable `Service`. Fails to compile before the fix (the two
`Either` branches had different error types); passes now that they are unified
to `BoxError`.
`option_layer` no longer constructs `Identity`, so the import is unused. Also
bring `Service` into scope in the regression test so `.call()` resolves.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Either rework broke option_layer

1 participant