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

Commits on Dec 2, 2023

  1. 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.
    connec committed Dec 2, 2023
    Configuration menu
    Copy the full SHA
    9e9827f View commit details
    Browse the repository at this point in the history

Commits on Dec 23, 2023

  1. 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).
    connec committed Dec 23, 2023
    Configuration menu
    Copy the full SHA
    84a49ae View commit details
    Browse the repository at this point in the history
  2. 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.
    connec committed Dec 23, 2023
    Configuration menu
    Copy the full SHA
    1cec0ae View commit details
    Browse the repository at this point in the history