search

ironthree / dxr

Declarative XML-RPC in Rust
Apache License 2.0
17 stars 9 forks source link

server_axum: Move addr to serve fn, add listener #6

Closed bahlo closed 1 year ago

bahlo commented 1 year ago

First of all: Thanks for creating the library, really happy with it so far 💯

This moves the addr out of the arguments for from_route and into serve. It also adds a new fn to start the server: serve_listener which allows starting the server with an existing TcpListener.

serve now creates the TcpListener (and panics in the same way bind would've panic'ed) and calls serve_listener.

I know this is a breaking change. Personally I think the addr belongs into serve but, happy to make any changes if you prefer a different solution.

decathorpe commented 1 year ago

Thanks for the PR, I'll take a closer look later. Though the CI already caught some instances where tests now fail to compile because they weren't updated for the API changes (there should be annotations in the "Files changed" tab). Everything else looks good at a first glance, thanks for taking care of updating the docs and README :)

PS: I'm not sure I like explicitly panic'ing - the method already returns a Result, and I didn't realize that bind can panic (guess I should've read the axum/hyper docs more closely). In hindsight, it would've been better to use try_bind and bubble up failures to the caller instead of calling the panic'ing Server::bind ... but this is something that I can fixup with a non-breaking followup change (because the error type from Server::from_tcp and Server::try_bind is the same, i.e. hyper::Error, which is already the error type in the dxr methods).

PPS: There's no need to worry about making any breaking changes in "main" - it's already no longer incompatible with the v0.5 branch because of the restructuring of the crates (and I've been planning some other possibly breaking changes as well, like further reducing the number of crates, for example, by making the "axum" support a non-default feature of the "dxr_server" crate instead of a separate crate).

bahlo commented 1 year ago

Will take care of the tests, rg didn't show me more matches so I thought CI will catch any leftovers (and it did, yay).

And I agree re panic'ing—the only issue is that constructing a TcpListener can fail with a io::Error and that'd make things a bit more complicated. Happy to throw in an error enum that switches between that and a hyper error if you'd prefer that.

decathorpe commented 1 year ago

UUUGH. I hate that the "time" crates bump MSRV so frequently (and that the developers don't seem to consider dozens of people complaining about it a problem, see https://github.com/time-rs/time/discussions/535 ).

Looks like dxr needs to bump from 1.64 to 1.65 because time now requires rust 1.65+ 😩 In the future I'll probably check Cargo.lock into git so at least the CI won't be impacted (unless I explicitly run "cargo update" and commit the new Cargo.lock) ...

But your changes look good to me, thanks for the contribution. I'll fix the CI with a follow-up commit ...