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

Only run the log middleware once per request #662

Merged
merged 1 commit into from
Jul 31, 2020

Conversation

jbr
Copy link
Member

@jbr jbr commented Jul 26, 2020

Currently, nested applications like

async fn main() -> Result<(), std::io::Error> {
    tide::log::start();
    let mut app = tide::new();
    app.at("/").nest({
        let mut api = tide::new();
        api.at("/").get(|_| async { Ok("Hello, world") });
        api
    });
    app.listen("127.0.0.1:8080").await?;
    Ok(())
}

will log twice. This provides a simple mechanism to ensure that a particular middleware is only run once per request. This approach also supports reentrant routing. This approach also requires no sweeping changes to the tide middleware system to ensure only-once middleware behavior, and also allows the middleware to define the desired behavior if it has already run once for the request. For example, a more complex logging middleware might record the time spent in each nested application but only emit that aggregated information once per request

Description

Introduces a private unit struct which is added to request extensions.

How Has This Been Tested?

added a test in tests/log.rs that uses logtest to assert that only one request received log entry is emitted, and only one response sent message is emitted per request/response cycle, even with a nested app. this test previously failed

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

closes: #551

This same pattern should be applied to the cookie middleware as well if this is merged

@jbr jbr marked this pull request as ready for review July 27, 2020 00:14
Copy link
Member

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, neat use of ext.

I do wonder if the cookies middleware should also do this? tbd in another PR I suppose.

@jbr
Copy link
Member Author

jbr commented Jul 28, 2020

@Fishrock123 I started to do this for the cookies middleware but couldn't figure out how to write a failing test. I think the issue is solved already for the cookies middleware because it reuses CookieData on request.ext and drains the cookie events on the response, so it can be run any number of times

Copy link
Member

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, this is great! 💯

@yoshuawuyts yoshuawuyts merged commit ed7ea86 into http-rs:master Jul 31, 2020
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.

Tide logging issue with nested routes — needs investigation
3 participants