aiohttp icon indicating copy to clipboard operation
aiohttp copied to clipboard

Fix 'I/O operation on closed file' and 'Form data has been processed already' upon redirect on multipart data

Open GLGDLY opened this issue 5 months ago • 5 comments

What do these changes do?

Fix 'I/O operation on closed file' and 'Form data has been processed already' upon redirect on multipart data, based on the discussion in #6853 .

This approach tries to pre-build Payload object for the data passing into the request, before the redirect while True loop starts, so we can reuse the same Payload object for the entire redirect chain. However, it is notable that I/O object is always an issue, so my thought here is to use the seek operation on the I/O object to allow chunk writing on redirect requests.

Yet, for non-seekable I/O objects, some possible solution are:

  • sending expect 100 first
  • local buffering
  • raise error

I think more discussion might be needed for non-seekable objects, so I just raise error in this PR first.

Another thing I think worth discussion is that I removed the close() operation on the I/O object in this PR due to the following reasons:

  • StringIOPayload do not close its I/O value in the original code,
  • Closing of the I/O value will make the payload un-reusable, even if it is seekable
  • We are not the one to create the I/O value, so maybe we should leave for user to close it.
  • standard I/O object will be closed upon destructure, so it won't affect the user interface

But I do think more discussions might be needed here.

Are there changes in behavior for the user?

No.

Is it a substantial burden for the maintainers to support this?

Yes.

Related issue number

Fixes #5577 Fixes #5530

Checklist

  • [x] I think the code is well written
  • [x] Unit tests for the changes exist
  • [x] Documentation reflects the changes
  • [x] 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.
  • [x] Add a new news fragment into the CHANGES/ folder
    • name it <issue_or_pr_num>.<type>.rst (e.g. 588.bugfix.rst)

    • if you don't have an issue number, change it to the pull request number after creating the PR

      • .bugfix: A bug fix for something the maintainers deemed an improper undesired behavior that got corrected to match pre-agreed expectations.
      • .feature: A new behavior, public APIs. That sort of stuff.
      • .deprecation: A declaration of future API removals and breaking changes in behavior.
      • .breaking: When something public is removed in a breaking way. Could be deprecated in an earlier release.
      • .doc: Notable updates to the documentation structure or build process.
      • .packaging: Notes for downstreams about unobvious side effects and tooling. Changes in the test invocation considerations and runtime assumptions.
      • .contrib: Stuff that affects the contributor experience. e.g. Running tests, building the docs, setting up the development environment.
      • .misc: Changes that are hard to assign to any of the above categories.
    • Make sure to use full sentences with correct case and punctuation, for example:

      Fixed issue with non-ascii contents in doctest text files
      -- by :user:`contributor-gh-handle`.
      

      Use the past tense or the present tense a non-imperative mood, referring to what's changed compared to the last released version of this project.

GLGDLY avatar Sep 19 '24 17:09 GLGDLY