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

[WIP] Complete chunked request on server eof #5238

Closed
wants to merge 1 commit into from

Conversation

greshilov
Copy link
Contributor

@greshilov greshilov commented Nov 14, 2020

What do these changes do?

Possible fix #5220

Code is hacky and questionable, hope maintainers will help.

Are there changes in behavior for the user?

No.

Related issue number

#5220

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> for example (588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

@CLAassistant
Copy link

CLAassistant commented Nov 14, 2020

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Nov 15, 2020

Codecov Report

Merging #5238 (5bcd59d) into master (a2fef0f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5238   +/-   ##
=======================================
  Coverage   97.54%   97.54%           
=======================================
  Files          43       43           
  Lines        8794     8796    +2     
  Branches     1413     1414    +1     
=======================================
+ Hits         8578     8580    +2     
  Misses        101      101           
  Partials      115      115           
Flag Coverage Δ
unit 97.39% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
aiohttp/client_reqrep.py 97.38% <100.00%> (+<0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a2fef0f...5bcd59d. Read the comment docs.

@luochen1990
Copy link

Hello, is this PR ready to be merged?

@Dreamsorcerer
Copy link
Member

@greshilov Want to revive this PR?
From what I can tell from asevtlov's comment (#5220 (comment)) this looks reasonable, just looks like we need something on the server side as well.

@thehesiod
Copy link
Contributor

@alexmac this may prove interesting

Dreamsorcerer added a commit that referenced this pull request Nov 3, 2023
Fixes #5220.

I believe this is a better fix than #5238. That PR detects that we
didn't finish sending a chunked response and then closes the connection.
This PR ensures that we simply complete the chunked response by sending
the EOF bytes, allowing the connection to remain open and be reused
normally.
@Dreamsorcerer
Copy link
Member

Superseded by #7764 .

Dreamsorcerer added a commit that referenced this pull request Nov 3, 2023
Fixes #5220.

I believe this is a better fix than #5238. That PR detects that we
didn't finish sending a chunked response and then closes the connection.
This PR ensures that we simply complete the chunked response by sending
the EOF bytes, allowing the connection to remain open and be reused
normally.

(cherry picked from commit 9c07121)
Dreamsorcerer added a commit that referenced this pull request Nov 3, 2023
Fixes #5220.

I believe this is a better fix than #5238. That PR detects that we
didn't finish sending a chunked response and then closes the connection.
This PR ensures that we simply complete the chunked response by sending
the EOF bytes, allowing the connection to remain open and be reused
normally.

(cherry picked from commit 9c07121)
xiangxli pushed a commit to xiangxli/aiohttp that referenced this pull request Dec 4, 2023
Fixes aio-libs#5220.

I believe this is a better fix than aio-libs#5238. That PR detects that we
didn't finish sending a chunked response and then closes the connection.
This PR ensures that we simply complete the chunked response by sending
the EOF bytes, allowing the connection to remain open and be reused
normally.

(cherry picked from commit 9c07121)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided There is a change note present in this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Server returns 400: "invalid character in chunk size header" for valid request
5 participants