Request for Code Review: Rust Web Server Project

code review
1

Hi everyone,

I've been working on a web server project in Rust and would greatly appreciate feedback from the community. The project's source code can be found on GitHub: https://github.com/evgenyigumnov/cblt.

I'm looking for constructive feedback on various aspects of the code, including but not limited to:

  1. Code Structure: Are there improvements to be made in the organization of modules and functions?
  2. Performance: Are there any obvious or potential bottlenecks? Suggestions for memory management optimizations would be valuable.
  3. Memory Management: How efficient is the handling of data, especially considering Rust's ownership and borrowing model?
  4. Error Handling: Are there better practices for handling errors that I should be using?
  5. Concurrency: Are the concurrency primitives used effectively, or could there be more efficient approaches?
  6. Code Readability: Is the code easy to follow? Any recommendations for clearer or more idiomatic Rust patterns?
  7. Security: Are there any security vulnerabilities or risks in the current implementation?
  8. Testing: Are the test cases comprehensive and effective? How could the test coverage be improved?

Any additional insights or suggestions are also welcome. Thank you in advance for your time and expertise!

1 Like
2

No replies :frowning:

Code base is perfect?

3

I guess nobody saw the thread the first time or wanted to comment. You have a pretty long review now.

Use cargo clippy. I get 28 warnings. Adding a #![warn(clippy::pedantic)] shows 95, although some might be false positives (it’s subjective what’s “good code”, but that’s why it’s called pedantic)

Grammar (in docs) and syntax variations

In line 13 of publish.sh there’s a comment in Russian. “Параметры релиза” (Google says it translates to “release parameters”) It’s fine to use whatever language you want, but all the rest of the code is in English.

In line 37 of the README it says “Voulmes” instead of “Volumes”.

CbltError::HeapLessError is constructed in multiple places with unnecessary brackets (CbltError::HeapLessError {}). Also I think the style would be to call it HeaplessError (the crate isn’t heap-less, it’s heapless). “Less” isn’t a separate word in “heapless”, it’s a suffix meaning “without”. (probably)

Potential Bugs

At line 99 of main.rs, you delete the ./reload file, unwrap()ing the result. You check that the file .exists(), but this is a TOCTOU (time-of-check time-of-use) issue. Something could delete that file before you get to it and bring down the program with a panic!(). Of course this shouldn’t happen, but it can. You can just return the error with ?.
There’s also another unwrap() on load_servers_from_config() nearby, which will panic!() if reloading is enabled and the Cbltfile is removed or contains incorrect syntax (and “incorrect syntax” includes the asset path etc. being longer than STRING_CAPACITY.

Is the sleep in line 101 just meant to provide an await point? Try tokio::task::yield_now.

Fixing the stack overflow

All the heapless types put every string on the stack. The strings in the program are all a constant STRING_CAPACITY (200) which is bigger than 99% of strings used. Just use a normal String or maybe something with the small string optimization (SSO).

Pedantic clippy warns that the future returned from process_workers is 108 KiB on the stack. That size comes from cblt::server::ServerSettings, which is ~32 KiB! The hosts field is a heapless::FnvHashMap with 8 buckets, each containing an array of 10 Directives. Directive is about 400 bytes (2x STRING_CAPACITY), 400 * 80 = 32,000 bytes (The Server struct also contains an identical field).

Edit: the dropdowns are probably unnecessary (the post looked much longer while I was writing it)

1 Like
4

Thank you! I will make corrections according to your comments in the nearest future. I used heapless to avoid allocations and not to use mutexes by cloning instead of taking locks. I think I will change it to regular strings and to avoid problems with locks I will use read and write locks. For me it rarely happens to write - this is only a case of reloading the server config.

5

I did fixes.

Except: 1 second sleep. It is need for decrease load on file system.

Could you check my improvements?