cargo-crev review

[dpc]

I am looking for a http request crate that would be tiny (LoC-wise), comparing to reqwest, with ability to disable anything other than the simplest plain http, suitable for security-concious environments (like querying BitcoinCore RPC port, etc.) . I was suggested ureq, so I decided to review the source.

Here's the result: dpc/crev-proofs@42a3b5c

Sorry for giving a negative review, but the point of reviewing is to judge and point out problems. I also reserve a right to be wrong about some parts. :)

I hope at least it will help you improve some stuff.

[lolgesten]

Thanks! I appreciate the feedback! I'll process it in a bit when I get some time.

[algesten]

Hi @dpc, thanks again for the review! Some very good observations and feedback. I've commented on the review here:

dpc/crev-proofs@42a3b5c

It resulted in a number of improvements. Thanks!

There are some overall API decisions done early on in the project (avoiding Result in favor of "synthetic errors", the funky builder pattern), which I perhaps wouldn't do today. I'm toying with the idea of doing another take of this lib revisiting those decisions (maybe basing it more on the standard http crate).

There's an underlying issue around *mut Stream currently required to reclaim the stream after using it in stuff like ChunkedDecoder.

I can see a couple of solutions: Ask for changes upstream to provide .into_inner() to get the underlying read/write back or clone the code into ureq and amend as needed.

I certainly don't trust myself to get raw pointers right, but I did spend some time fussing over it. Given the explanation as to why they are there, I would obviously appreciate any concrete thoughts on how I'm misusing them.

Maybe this project in its current form can never reach the standard you're looking for to give it a neutral to positive review. However I'm keen to know what it would take.

[algesten]

A follow up comment. It was quite some time since I wrote this, and I had to retrace the steps in my brain as to why I think my unsafe use might be ok.

struct PoolReturnRead (but the same goes for ReclaimingRead) has the raw pointer as a struct field. That means rust will mark these structs as not thread safe (it won't auto derive Send).

So the struct, once created, will be forced by rust to stay on the current thread.

Returning the pooled connection can happen in two ways – either by reaching end-of-stream or by drop trait.

The guard that pool return happens only once is not thread safe, but the struct is locked to the thread already.

The wrapped reader, which currently "owns" the stream we try to bring back, is first removed.

Unless there's some funky stuff going on in places like ChunkedDecoder, this will deallocate both that decoder the ReclaimedRead it's wrapping, leaving only the *mut Stream we have here.

So bringing the stream back should at this point be ok.

Granted, I do not trust myself here.