HTTP server based on `mio::TcpListener` that can actually be stopped

After discussion here, I have re-written my "demo" HTTP server with mio::TcpListener :smirk:

The server can actually be shut down "cleanly" when SIGINT is retrieved.

Also, because there seems to be no other/better way with mio::TcpListener, we keep on reading the request until either a zero is read encountered, or the \r\n\r\n end marker was seen.

The server does not support HTTP/1.1 Pipelining, but browser support seems to be lacking anyway.

Opinions? Looks reasonable?

Code:

Thanks for any feedback and suggestions!

A few tiny nits:

  • You can replace "127.0.0.1".parse().unwrap() with std::net::Ipv4Addr::LOCALHOST.into() to avoid runtime parsing and unwrapping. (Though I imagine you will probably want std::net::Ipv4Addr::UNSPECIFIED when deploying.)
  • It's not clear to me what the match listener.canceller() expression is doing in Server::canceller. Why not replace this match expression with just listener.canceller()?
  • The implementation of Server::thread_main is kind of hard to parse (at least for me). It might benefit from some let Ok(_) = _ else { return ... }; statements or .map_err(...)?s.
  • It might be helpful for other people reading the code to annotate atomic accesses with why certain orderings were chosen.

That's mostly style-wise. I can't really comment on the correctness or design as I haven't looked at the code long enough.

Thanks for feedback!

It's not clear to me what the match listener.canceller() expression is doing in Server::canceller. Why not replace this match expression with just listener.canceller()?

Yeah, I think it's redundant and can be removed :slight_smile:

The implementation of Server::thread_main is kind of hard to parse (at least for me). It might benefit from some let Ok(_) = _ else { return ... }; statements or .map_err(...)?s.

My feeling is that a match expression is usually preferred over if let...s + else, provided that we have more than one "case" to handle, because it leads to more condensed code.

It might be helpful for other people reading the code to annotate atomic accesses with why certain orderings were chosen.

I don't claim that I understand atomic memory ordering in full details :sweat_smile:

My understanding is that we should use "Release" if we write the atomic value; "Accquire" if we read the atomic value; and "AcqRel" to update (read + possibly write) the atomic value, as in compare-exchange.

Yeah, using acquire release when you're not synchronizing memory via the flag is confusing. Relaxed is sufficient here.

Updated with various improvements!

Orderings affect how the atomic memory access relates to other memory accesses (atomic or otherwise). For example, you'd use Release if you were updating an atomic to signal that you had finished writing other memory (e.g. releasing a lock) and Acquire before reading that memory (acquiring the lock).

If you don't care about consistency with accesses to any other memory locations, which is usually the case for a cancellation flag (you only care that it eventually reads as true ) then you can use Relaxed for every access.

2 Likes

I just briefly skimmed it, but one tiny nit is that, if you're not using the const_format crate for anything else, why not replace your call to formatcp with a simple concat expression from the standard library? Unless you're planning to do something more complicated.

Because I didn't know that (or how) concat can be used with const values and only found const_format as a workaround :sweat_smile:

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.