search

drogue-iot / reqwless

Rust async HTTP client for embedded/no_std
Apache License 2.0
129 stars 18 forks source link

Implement BufRead for BodyReader #45

Closed bugadani closed 1 year ago

bugadani commented 1 year ago

This PR allows users to read from a TLS connection without copying. Non-TLS sessions will be able to save some memory by reusing the initial read buffer that was passed in to store the HTTP headers.

bugadani commented 1 year ago

This might be a bit on the complex side as I had to replace the whole concatenating reader concept with a buffering one. But I also couldn't use buffered io because it doesn't support preloaded content that we have after reading headers.

The idea is that, if we have a buffered connection (via embedded-tls), we use its buffer. Otherwise, we read into the rx buffer the user provided while sending the request. We use these buffers as the backing storage of BufRead.

I've reimplemented the higher-level regular reads in terms of this buffering. This means that the normal read calls may copy twice (once to the internal buffer, once to the output), which isn't ideal.

bugadani commented 1 year ago

I've reimplemented the higher-level regular reads in terms of this buffering. This means that the normal read calls may copy twice (once to the internal buffer, once to the output), which isn't ideal.

@lulf @rmja do you have opinions on this? I can resolve this by some not-so-elegant code around the body readers and the new buffered reader here (i.e. by introducing a buffered_len() fn that I could branch on and read into the output directly if empty).

Though to be honest I probably wouldn't want to spend much time on this PR, this could be resolved later.

bugadani commented 1 year ago

I haven't used these new APIs

The question is more that this PR makes the old API slightly slower :)

rmja commented 1 year ago

Is it correctly understood that the underlying non-tls connection now must implement BufRead? Could this be more a conditional constraint such that the BodyReader implemented BufRead if the underlying connection did?

bugadani commented 1 year ago

Is it correctly understood that the underlying non-tls connection now must implement BufRead?

No and if that's currently the case I did something wrong. This PR is supposed to emulate BufRead by reading into the provided rx buffer.

Unfortunately, I don't think it's possible to specialize for a BufRead TcpConnection.

Update: I can see how the tests might have been misleading. I've removed BufRead from the test buffer.