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

Send 100 Continue when ready to receive topic downstream #1010

Merged
merged 4 commits into from
Jan 11, 2024

Conversation

patuwwy
Copy link
Member

@patuwwy patuwwy commented Jan 3, 2024

What?

🔷 Disable auto sending HTTP 100 Continue

NodeJs auto sends HTTP 100 Continue when it is not strictly handled on server.
We handle it in other way and send when needed so it is sent twice.

https://nodejs.org/api/http.html#event-checkcontinue
https://github.com/nodejs/node/blob/v18.x-staging/lib/_http_server.js#L1110

🔷 Topic downstream queue update

  • Require expect: 100-continue header in topic downstream endpoint.
  • Send HTTP 100 Continue when request can be processed.

🔷 Refactoring

Move TopicRouter / ServiceDiscovery types to @scramjet/types


❗ User should wait for HTTP 100 Continue and then start to upload data.

@scramjet/cli / @scramjet/api-client CAN'T listen for 100 Continue as we use node-fetch which doesn't support continue request's event. Data will be sent immediately.

If data are sent with no wait for 100 Continue some bytes can be lost when request is interrupted before we started handling these data.


Review checks:

These aspects need to be checked by the reviewer:

  • Verify and confirm operation (please post a screenshot)
  • All STH tests pass
  • All Scramjet Cloud Platform tests pass
  • Documentation is updated or no changes

@patuwwy patuwwy force-pushed the fix/auto-100-continue branch 9 times, most recently from 7fd125c to f1c2693 Compare January 4, 2024 14:05
@patuwwy patuwwy changed the title Disable auto writing 100 Continue Send 100 Continue when ready to receive topic downstream Jan 4, 2024
@patuwwy patuwwy requested a review from MichalCz January 8, 2024 11:54
@patuwwy patuwwy force-pushed the fix/auto-100-continue branch 6 times, most recently from a67f9eb to 96ed88d Compare January 9, 2024 05:30
@a-tylenda
Copy link
Contributor

verified

image

@a-tylenda
Copy link
Contributor

image

@a-tylenda a-tylenda merged commit 7eada46 into devel Jan 11, 2024
19 checks passed
@a-tylenda a-tylenda deleted the fix/auto-100-continue branch January 11, 2024 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants