Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support reading in chunks #160

Closed
cryi opened this issue Aug 11, 2023 · 10 comments
Closed

Support reading in chunks #160

cryi opened this issue Aug 11, 2023 · 10 comments

Comments

@cryi
Copy link
Contributor

cryi commented Aug 11, 2023

Hello,

I've noticed that corehttp currently doesn't have a feature for reading the body in chunks. While examining the source, particularly the httpParserOnBodyCallback section, I'm curious if it would be possible to introduce a mechanism that allows the body to be read in distinct chunks.

The envisioned callback could return:

  • A value less than 0 in case of an error.
  • A value greater than 0 to indicate the number of bytes processed or written to the buffer.
  • A return value of 0 to indicate that all is well, but no further action is needed.

By enabling chunked reading, users can easily manage large responses or work with limited buffer sizes more effectively.

I'd love to hear your thoughts on this suggestion.

Thank you!

P.S.: I am integrating corehttp as a basic replacement for libcurl. I'm uncertain about the extent of support for range headers. Thus, I'm exploring potential solutions.

@cryi cryi changed the title Support for Simple Streaming in corehttp Support reading in chunks Aug 11, 2023
@cryi
Copy link
Contributor Author

cryi commented Aug 11, 2023

Hmm, when I think about it, maybe I could check for the corehttp status. If it says 'out of buffer', I could check the Content-Length or Transfer-Encoding: chunked and continue reading the body directly from the socket.🤔

@cryi
Copy link
Contributor Author

cryi commented Aug 13, 2023

Yes, it functions as anticipated. We can directly dump content from the socket based on content-length. Technically, there's no need for adjustments in corehttp. However, I'll keep this open for further discussion, in case you consider integrating it into the library

@cobusve
Copy link
Member

cobusve commented Aug 24, 2023

Looks like an important addition to make, any chance you could turn your suggestion into a PR to update the library?

@cryi
Copy link
Contributor Author

cryi commented Aug 25, 2023

Hi @cobusve, Possibly I can. Would be something like

HTTPStatus_t HTTPClient_Read(const TransportInterface_t * pTransport, const uint8_t * buffer,  size_t bytesToRead) { ... }

suitable?

@archigup
Copy link
Member

A function with that signature sounds good

@cryi
Copy link
Contributor Author

cryi commented Sep 24, 2023

Ok I should get time to take a look on this some time in October

@cryi
Copy link
Contributor Author

cryi commented Oct 12, 2023

Hi, I've created a PR #165 with a sample implementation, although I'm uncertain about the naming convention. Do you have any suggestions for improvement?

Regarding the implementation, could you review it and ascertain if this would be suitable? If not, we can close the PR and proceed with the implementation on our side. Currently, the suggested approach has a downside: developers using coreHTTP need to check for the beginning of the body in the response buffer and, based on the content length, read the remainder through HTTPClient_Read. Thus, the entire body is not directly readable.

I've tested this with our app, which we are migrating from libcurl, and it seems to work well. Of course, I'll add the necessary tests and documentation as required.

@cryi
Copy link
Contributor Author

cryi commented Oct 13, 2023

Here is how we use it where preloadedBytesToUse are bytes stored in original response buffer.

    HTTPBody_t body = {0};
    body.pBuffer = buffer + preloadedBytesToUse;
    body.bufferLen = bufferLen - preloadedBytesToUse;

    HTTPStatus_t returnStatus = HTTPClient_Read(context->transport, &body);
    if (returnStatus != HTTPSuccess) {
        return push_error(L, "Failed to read response body.");
    }
    // here we got body bytes in `pBuffer` with length of `body.bodyLen`

@cryi
Copy link
Contributor Author

cryi commented Oct 16, 2023

This approach is not ideal. I am working on better one, Feel free to ignore this.

@cryi
Copy link
Contributor Author

cryi commented Oct 25, 2023

Superseded by #166

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants