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

Add Ability to Stop Parsing After HTTP Headers #172

Merged
merged 11 commits into from
Jan 29, 2024

Conversation

cryi
Copy link
Contributor

@cryi cryi commented Jan 17, 2024

Add Ability to Stop Parsing After HTTP Headers

Description

This pull request adds the ability to stop parsing immediately after HTTP headers. This feature is beneficial for handling chunked encoding scenarios. Prior to this update, llhttp would automatically begin parsing the body, which lead to issues with tracking chunks and their respective sizes in cases where buffer capacity is exceeded.
This PR addresses the issue by introducing HTTP_RESPONSE_DO_NOT_PARSE_BODY_FLAG, which halts parsing after the headers and stores any remaining data in the body buffer. The subsequent processing is intended to be handled by a higher-level library.

Test Steps

With the implementation of patches from #167, processing responses becomes possible even when the initial buffer capacity is exceeded. However, without the HTTP_RESPONSE_DO_NOT_PARSE_BODY_FLAG, processing chunked responses in this manner is not achievable.

Checklist:

  • I have tested my changes. No regression in existing tests.
  • I have modified and/or added unit-tests to cover the code changes in this Pull Request.

Related Issue

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

cryi and others added 2 commits January 17, 2024 22:43
* format
---------

Co-authored-by: GitHub Action <[email protected]>
@cryi
Copy link
Contributor Author

cryi commented Jan 17, 2024

Hi @archigup @ericbj29,

hope you're both doing well! I wanted to bring your attention to this PR, which includes formatting and some initial documentation. I'd really appreciate it if you could take a moment to look it over. Thanks a ton in advance! 🌟

Edit: I've enabled editing for maintainers, so feel free to make any adjustments you think are necessary.

@ericbj29
Copy link

Hi @cryi, thank you for opening this pull request. We'll take a look and get back to you.

n9wxu
n9wxu previously approved these changes Jan 18, 2024
source/core_http_client.c Outdated Show resolved Hide resolved
@AniruddhaKanhere
Copy link
Member

/bot run uncrustify

@cryi
Copy link
Contributor Author

cryi commented Jan 19, 2024

Hey @ericbj29, closed by accident?

@ericbj29
Copy link

Yes. Only meant to re-trigger the CI checks. Seems I accidentally closed the PR with my force push

@cryi
Copy link
Contributor Author

cryi commented Jan 19, 2024

Can you reopen? Looks like I am not able to do that.

@AniruddhaKanhere
Copy link
Member

@cryi, Yes, we accidentally removed the commits. Thus the PR cannot be opened. We are working on fixing it.

However, I cannot commit to your branch - is there anything that changed? Can you add me as a collaborator please to your repository?

@AniruddhaKanhere
Copy link
Member

AniruddhaKanhere commented Jan 19, 2024

To that end, would you mind accepting this PR? cryi#4. It would restore all the commits made by you and us. Thanks

EDIT: After you accept this PR to your repository, we can reopen this PR. Thanks for understanding - we apologize for all the confusion.

@cryi
Copy link
Contributor Author

cryi commented Jan 19, 2024

@AniruddhaKanhere merged. No worries. Regarding what changed - I can not give you access to the branch while PR is closed. The access automatically resets itself.

@AniruddhaKanhere
Copy link
Member

/bot run uncrustify

Skptak
Skptak previously approved these changes Jan 28, 2024
@Skptak Skptak dismissed their stale review January 28, 2024 18:22

Failing CBMC Proof

Signed-off-by: Gaurav Aggarwal <[email protected]>
@aggarg
Copy link
Member

aggarg commented Jan 29, 2024

@cryi Thank you for your contribution. It seems that with this change, you are able to handle chunked responses. One other community member is interested in the same - https://forums.freertos.org/t/how-to-enable-transfer-encoding-chunked-respose/18909/3. Would you be willing you share your code for handling chunked responses?

Also, if you share that code, we can decide if we should add this functionality in this library itself (thereby saving you from maintaining another library).

@AniruddhaKanhere
Copy link
Member

@cryi thank you for your contribution. I'll be merging this PR now.

@AniruddhaKanhere AniruddhaKanhere merged commit 65eba45 into FreeRTOS:main Jan 29, 2024
11 checks passed
@cryi
Copy link
Contributor Author

cryi commented Jan 29, 2024

Hi @aggarg, it is public library lua-corehttp.

I tried to post relevant parts to the freertos forum but it stopped me with "new users can not post links". So here it is:

I do not think you should add it into @aggarg. Like you could but I am pretty sure that would mean a major version upgrade. I would need more hooks to get data from chunked response processing on corehttp/llhttp side. It would likely mean harder control flow on my side too.

I tried to do this PR through hooks initially. But it proved to be limiting and not that readable/nice. Like now on high level in our lua engine we can read both chunked and body response in simple loops and optionally control the behavior - like in the linked we postprocess data with deflate if needed.

With this PR stop reading at the end of chunked response is the only hard part. With that figured you just read with HTTPClient_Read raw data as needed. So including in the corehttp may not be necessary.

@aggarg
Copy link
Member

aggarg commented Jan 30, 2024

@cryi Thank you sharing the code and detailed response.

@cryi
Copy link
Contributor Author

cryi commented Jan 31, 2024

No problem, @aggarg. Just a heads-up: we currently have the capability to handle SSE too. However, if you implement a chunked response and omit the elements from #172 and #167, we might risk losing this functionality.

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

Successfully merging this pull request may close these issues.

8 participants