Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade axum to 0.7 #34

Merged
merged 3 commits into from
Dec 23, 2023
Merged

Upgrade axum to 0.7 #34

merged 3 commits into from
Dec 23, 2023

Conversation

connec
Copy link
Member

@connec connec commented Dec 13, 2023

  • 9e9827f chore: upgrade axum to 0.7

    The largest changes are caused by the associated upgrade of hyper to
    1.0, including the removal of axum::Server (replaced by axum::serve)
    and http body utilities (replaced by http-body-util). These changes
    were purely mechanical.

    A more subtle change is the requirement from http 1.0 for request
    extensions to be Clone. The Lazy transaction wrapper is not Clone,
    so it has been wrapped in an Arc, with Arc::get_mut being used to
    obtain the necessary mutability. This feels like it might be buggy,
    since other middleware could potentially clone and store request
    extensions, without otherwise interfering with the transaction (meaning
    the Tx extractor should still work). This needs subsequent
    investigation before release.

  • 84a49ae fix: fix buggy behaviour when cloning request extensions

    Our naive update for [email protected] / [email protected] led to buggy behaviour whereby
    the Tx extractor would always fail with OverlappingExtractors if
    there were any outstanding clones of the request extensions (see the new
    test). Since request extensions now must all implement Clone, it's
    possible that some middleware might wish to keep a clone of all request
    extensions (e.g. for request inspection/tracing/debugging), rendering
    Tx unusable with those middleware.

    To fix it, we simplify the synchronisation by implementing Clone for
    Slot and creating new Extension<DB> and LazyTransaction<DB> types
    to replace TxSlot<DB> and Lazy<DB>.

    Slot<T> is a wrapper around an Arc<Mutex<Option<T>>>, and as such it
    can trivially implement Clone (there was some "pit of success"
    considerations with the previous API intended to enforce proper usage,
    but that is unnecesarily limiting given the underlying Mutex).

    The Extension<DB> holds a Slot containing a LazyTransaction<DB>.
    Extension<DB> is trivially clonable since Slot itself is. The
    LazyTransaction<DB> then implements a simple "lazily acquired
    transaction" protocol, making use of normal rust ownership and borrowing
    rules to manage the transaction (i.e. it has no internal
    synchronisation).

    This makes the overall synchronisation picture much simpler: the
    middleware future and all clones of the request extension hold a
    reference to the same Slot. The Tx extractor obtains its copy of the
    request extension and attempts to lease the inner LazyTransaction,
    failing with OverlappingExtractors if the lease is already taken (this
    is the only public invocation of lease, and so overlapping extractors
    can be the only* cause of an absent transaction). If the lease is
    successful, the extractor can acquire a transaction (if there's not one
    already) and package it up for request handlers to then interact with.

    • Technically the transaction can be "stolen" from the Tx extractor by
      committing explicitly, but this considered to create an endless
      "overlap" in the current semantics.

    The main caveat of this approach seems to be that the Tx extractor no
    longer has type-level knowledge that it can access a Transaction - the
    Transaction can only be accessed by matching on the LazyTransaction
    state. This doesn't affect the API, but it could make bumping into a
    panic more likely, or there may be performance implications (though
    these would likely be dwarfed by the I/O involved in interacting with a
    database).

  • 1cec0ae refactor: ditch slot

    It turns out the parking_lot arc_lock future does the main thing we
    want from Slot, giving us 'static lock guards that unlock the value
    on drop. The only missing functionality is the "stealing" we need to
    obtain ownership in order to commit the transaction. Rather than
    implementing this via Option, we add an additional state to
    LazyTransaction and handle it there.

    Ultimately this removes a lot of code, and makes the synchronisation
    mechanism even less exotic.

The largest changes are caused by the associated upgrade of hyper to
1.0, including the removal of `axum::Server` (replaced by `axum::serve`)
and http body utilities (replaced by `http-body-util`). These changes
were purely mechanical.

A more subtle change is the requirement from `http` 1.0 for request
extensions to be `Clone`. The `Lazy` transaction wrapper is not `Clone`,
so it has been wrapped in an `Arc`, with `Arc::get_mut` being used to
obtain the necessary mutability. This feels like it might be buggy,
since other middleware could potentially clone and store request
extensions, without otherwise interfering with the transaction (meaning
the `Tx` extractor should still work). This needs subsequent
investigation before release.
Our naive update for [email protected] / [email protected] led to buggy behaviour whereby
the `Tx` extractor would always fail with `OverlappingExtractors` if
there were any outstanding clones of the request extensions (see the new
test). Since request extensions now must all implement `Clone`, it's
possible that some middleware might wish to keep a clone of all request
extensions (e.g. for request inspection/tracing/debugging), rendering
`Tx` unusable with those middleware.

To fix it, we simplify the synchronisation by implementing `Clone` for
`Slot` and creating new `Extension<DB>` and `LazyTransaction<DB>` types
to replace `TxSlot<DB>` and `Lazy<DB>`.

`Slot<T>` is a wrapper around an `Arc<Mutex<Option<T>>>`, and as such it
can trivially implement `Clone` (there was some "pit of success"
considerations with the previous API intended to enforce proper usage,
but that is unnecesarily limiting given the underlying `Mutex`).

The `Extension<DB>` holds a `Slot` containing a `LazyTransaction<DB>`.
`Extension<DB>` is trivially clonable since `Slot` itself is. The
`LazyTransaction<DB>` then implements a simple "lazily acquired
transaction" protocol, making use of normal rust ownership and borrowing
rules to manage the transaction (i.e. it has no internal
synchronisation).

This makes the overall synchronisation picture much simpler: the
middleware future and all clones of the request extension hold a
reference to the same `Slot`. The `Tx` extractor obtains its copy of the
request extension and attempts to `lease` the inner `LazyTransaction`,
failing with `OverlappingExtractors` if the lease is already taken (this
is the only public invocation of `lease`, and so overlapping extractors
can be the only* cause of an absent transaction). If the lease is
successful, the extractor can acquire a transaction (if there's not one
already) and package it up for request handlers to then interact with.

* Technically the transaction can be "stolen" from the `Tx` extractor by
committing explicitly, but this considered to create an endless
"overlap" in the current semantics.

The main caveat of this approach seems to be that the `Tx` extractor no
longer has type-level knowledge that it can access a `Transaction` - the
`Transaction` can only be accessed by matching on the `LazyTransaction`
state. This doesn't affect the API, but it could make bumping into a
panic more likely, or there may be performance implications (though
these would likely be dwarfed by the I/O involved in interacting with a
database).
It turns out the `parking_lot` `arc_lock` future does the main thing we
want from `Slot`, giving us `'static` lock guards that unlock the value
on drop. The only missing functionality is the "stealing" we need to
obtain ownership in order to commit the transaction. Rather than
implementing this via `Option`, we add an additional state to
`LazyTransaction` and handle it there.

Ultimately this removes a lot of code, and makes the synchronisation
mechanism even less exotic.
@connec connec marked this pull request as ready for review December 23, 2023 21:01
@connec connec merged commit 9a2f761 into master Dec 23, 2023
19 checks passed
@connec connec deleted the axum-0.7 branch December 23, 2023 21:01
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.

1 participant