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.

Sign up for GitHub

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

Jump to bottom

Handling Expect-100 Continue header #3200

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

Handling Expect-100 Continue header #3200

wants to merge 6 commits into from

Conversation

dhavalsingh
Copy link
Contributor

@dhavalsingh dhavalsingh commented Jul 31, 2023 •
edited

…gins reading the body

Description

Please describe your pull request. Thank you for contributing! You're the best.

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added (or updated) appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.

dhavalsingh
dhavalsingh commented Jul 31, 2023
View reviewed changes
lib/puma/client.rb Outdated
# going forward
@io << HTTP_11_100
@io.flush
# Determine if a redirect is needed, this could be a function call that encapsulates
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to understand if this is what's expected

@dhavalsingh
Copy link
Contributor Author

Will clean up the PR once I understand I am on the right path to solving this issue.
@dentarg can you help as you created this issue?

@dhavalsingh dhavalsingh mentioned this pull request Jul 31, 2023
Open
@dentarg
Copy link
Member

dentarg commented Aug 1, 2023

This doesn't look correct to me. We want to application to define the response (e.g. the 307 redirect).

@dentarg
Copy link
Member

dentarg commented Aug 1, 2023

The best guidance I can give you is to look at how the Python HTTPServer does it (https://github.com/python/cpython/blob/3.12/Lib/http/server.py) as I have not researched this topic myself yet.

@dentarg
Copy link
Member

dentarg commented Aug 1, 2023

Probably good to drive the implementation using tests.

@dhavalsingh
Copy link
Contributor Author

The best guidance I can give you is to look at how the Python HTTPServer does it (https://github.com/python/cpython/blob/3.12/Lib/http/server.py) as I have not researched this topic myself yet.

Ah, okay. Will look into this. Thanks a lot!

@dhavalsingh
Copy link
Contributor Author

Where are client specific tests written? cant figure it out, or there arent any?

@dentarg
Copy link
Member

dentarg commented Aug 1, 2023

test/test_web_server.rb is probably a good fit for this

dhavalsingh
dhavalsingh commented Aug 2, 2023
View reviewed changes
lib/puma/request.rb Outdated
@@ -585,8 +585,10 @@ def str_headers(env, status, headers, res_body, io_buffer, force_keep_alive)
# reply with the proper 200 status without having to compute
# the response header.
#
if status == 200
if status == 200 && !expect_100_continue
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if this is the best way to handle this

dhavalsingh
dhavalsingh commented Aug 2, 2023
View reviewed changes
test/test_web_server.rb
@@ -99,6 +99,14 @@ def test_nonexistent_http_method
socket.close
end

def test_expect_100_header

This comment was marked as resolved.

Sign in to view

@dhavalsingh
Copy link
Contributor Author

dhavalsingh commented Aug 2, 2023 •
edited

Hey @dentarg,
I have made some changes and added a test. Can you check if I am in the right direction?
So, the expected behavior I want is:
For a Request with Expect-100 continue header => Send the request with just the header to the app => If the app responds with a non-200 status I should respond with that(unauthorized access, redirect...) for 200 status from the app I will respond with 100 (https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Expect) and then send the request body to the app to process.

I have 4 doubts:

  1. Am I correct in the above explanation?
  2. If the above understanding is correct, then that means all apps have to handle this right? They should be able to handle just the header.
  3. Do we need to handle the 417 response separately as mentioned in the MDN docs?
  4. What do you guys use for debugging the code? I used byebug in another branch for debugging but had to remove from the gemfile as it's not present in the master

@dhavalsingh dhavalsingh changed the title Added a redirect loging and moved the expect 100 response when app be… Handling Expect-100 Continue header Aug 2, 2023
@dhavalsingh
Copy link
Contributor Author

Also, whats the diff between test_puma_server vs test_web_server tests?

dentarg
dentarg reviewed Aug 2, 2023
View reviewed changes
lib/puma/client.rb
Comment on lines -345 to +349
# TODO allow a hook here to check the headers before
# going forward
@io << HTTP_11_100
@io.flush
@http_expect_100_continue_header_present = true
Copy link
Member

@dentarg dentarg Aug 2, 2023 •
edited

Choose a reason for hiding this comment

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

Pretty sure we don't want to remove the current behaviour of responding quickly to Expect: 100 Continue and that allowing the app to define the response should be something the user opt-in to (via the Puma config probably)

Copy link
Member

Choose a reason for hiding this comment

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

Agreed!

Copy link
Member

Choose a reason for hiding this comment

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

(Or at least, we should keep that behavior UNLESS the user/app says they want something else).

@nateberkopec
Copy link
Member

This seems like a fine place to start for me. Next step would be a config setting that allows the user to define what to do if a Expect: 100 is encountered.

@nateberkopec nateberkopec added the waiting-for-changes Waiting on changes from the requestor label Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dentarg dentarg dentarg left review comments

@nateberkopec nateberkopec nateberkopec left review comments

Assignees
No one assigned
Labels
feature waiting-for-changes Waiting on changes from the requestor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@dhavalsingh @dentarg @nateberkopec