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

fix(node:http) resume when reading and avoid unnecessary pause/resumes calls #18804

Merged
merged 20 commits into from
Apr 8, 2025

Conversation

cirospaciari
Copy link
Member

@cirospaciari cirospaciari commented Apr 6, 2025

What does this PR do?

Fix: #18799

  • Documentation or TypeScript types (it's okay to leave the rest blank in this case)
  • Code changes

How did you verify your code works?

Tests

@robobun
Copy link

robobun commented Apr 6, 2025

Updated 4:08 PM PT - Apr 7th, 2025

@cirospaciari, your commit 1270f4b has 1 failures in Build #14600:


🧪   try this PR locally:

bunx bun-pr 18804

@cirospaciari cirospaciari marked this pull request as ready for review April 6, 2025 00:57
Copy link
Collaborator

@Jarred-Sumner Jarred-Sumner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whether or not it's paused probably needs to go on the NodeHTTPSocket instead of the NodeHTTPResponse, right? Since the socket can outlive the response and we don't want to end up in a state where the previous response was paused and then the next request never happens

@cirospaciari
Copy link
Member Author

Whether or not it's paused probably needs to go on the NodeHTTPSocket instead of the NodeHTTPResponse, right? Since the socket can outlive the response and we don't want to end up in a state where the previous response was paused and then the next request never happens

The socket will always be resumed if outlived, is_paused is needed here because it direct uses usockets with do not track this state raw_response is a opaque struct, we can change this flag to be inside usockets with makes more sense

@cirospaciari cirospaciari marked this pull request as draft April 6, 2025 01:55
@cirospaciari cirospaciari marked this pull request as ready for review April 6, 2025 02:17
@cirospaciari cirospaciari marked this pull request as draft April 6, 2025 02:35
@cirospaciari cirospaciari force-pushed the ciro/fix-pause-resume branch from eb75a2f to 5cc1a1f Compare April 7, 2025 17:57
@cirospaciari cirospaciari force-pushed the ciro/fix-pause-resume branch from c56efa1 to bc59f6f Compare April 7, 2025 21:07
@cirospaciari cirospaciari marked this pull request as ready for review April 7, 2025 22:37
@cirospaciari cirospaciari merged commit a2efbd4 into main Apr 8, 2025
70 of 71 checks passed
@cirospaciari cirospaciari deleted the ciro/fix-pause-resume branch April 8, 2025 00:30
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.

request.formData() hangs in Astro app when deployed on ARM Linux
3 participants