sigaloid / vial

đŸ§ª a micro micro-framework for rust
http://vial.rs
Apache License 2.0
3 stars 0 forks source link

Request handling changes #9

Closed JEBailey closed 2 years ago

JEBailey commented 2 years ago

I took a different approach to your timeout mechanism and went with implementing a non-blocking stream. I then polled for the entire contents of the request before parsing.

The idea behind this was to prevent the parser from stopping the parsing process, kicking back the request for more data, and then restarting the process from the beginning. This works great but it resulted in a number of changes.

Since we have the full request from the beginning, there is no need for a http_parse::Status as there is never a partial status. Either we received the full request, or the connection was assumed to be closed prematurely. This changes some of the tests that were expecting a partial result versus a connection-closed.

The determination of whether the body was completely sent was moved into the parser.

Rather than relying on the caller of the Request to remember to set the stream to non-blocking, I added a new method to Request set that when parsing from the stream.

As part of this, I ended up restructuring the parser so that I could understand the parsing process better which is where I found a subtle problem with how it was handling the path validation. Which I clarified by adding a new ParsePath error.

This is large PR and I'm fairly new to Rust. So I'd appreciate any feedback.

sigaloid commented 2 years ago

That commit should fix tests. Will accept when passes :)

JEBailey commented 2 years ago

So it's been a year or two since I frequented GitHub. What black magic was it that allows you to modify my repository with the fix for the tests? That's neat and I haven't seen that before.

sigaloid commented 2 years ago

When creating a pull request, there's an option to allow edits from maintainers of the repo you are trying to merge into. I honestly forgot it existed and I was going to just create a PR in your fork.

I reran the tests as it seems like that one that failed the first time is a bit flaky - if it fails again it's probably the fault of the test for some reason even though it passed tests on my machine.

sigaloid commented 2 years ago

Yep, now it passes - that test must be unreliable since I reran it without edits and it passed. Merging!

thread 'body_len' panicked at 'assertion failed: `(left == right)`
  left: `200`,
 right: `404`', tests/response_test.rs:168:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

failures:
    body_len

test result: FAILED. 14 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s