Skip to content

Fix ProtocolError exceptions bypassing download resume logic#14084

Open
lets-build-an-ocean wants to merge 5 commits into
pypa:mainfrom
lets-build-an-ocean:fix/incomplete-read-resume
Open

Fix ProtocolError exceptions bypassing download resume logic#14084
lets-build-an-ocean wants to merge 5 commits into
pypa:mainfrom
lets-build-an-ocean:fix/incomplete-read-resume

Conversation

@lets-build-an-ocean

Copy link
Copy Markdown

When a download is interrupted mid-stream by a sudden network drop, urllib3 raises a ProtocolError (wrapping an IncompleteRead). Because _process_response() only caught ReadTimeoutError, the ProtocolError propagated uncaught, skipping the is_incomplete() check in __call__() and never reaching _attempt_resumes_or_redownloads(). This means pip's built-in resume logic was never triggered.

Fixes #14079

What I changed:

  • Added ProtocolError to the except clause in _process_response() so broken connections are handled the same as timeouts.
  • Added ProtocolError to the retry loop's except clause in _attempt_resumes_or_redownloads() so resume attempts that also drop don't crash either.
  • Updated the warning message to "Connection interrupted while downloading." since it now covers both timeouts and broken connections.

Testing:
Added a unit test that simulates a ProtocolError being raised mid-stream and verifies pip resumes and completes the download instead of crashing. Also verified manually using the reproduction script from the issue.

@sepehr-rs

Copy link
Copy Markdown
Member

Hi @lets-build-an-ocean, thank you for your contribution!
On a quick look and local testing, this looks good and I can confirm the exception is handled correctly now. Just a heads up that it may be a little while before a maintainer can take a closer look at this, since we're all volunteers. Appreciate your patience!

@lets-build-an-ocean

Copy link
Copy Markdown
Author

Hi @lets-build-an-ocean, thank you for your contribution! On a quick look and local testing, this looks good and I can confirm the exception is handled correctly now. Just a heads up that it may be a little while before a maintainer can take a closer look at this, since we're all volunteers. Appreciate your patience!

Thanks for the quick review and for confirming the fix works! No rush at all — I really appreciate the time you and the other volunteers put into this project.

@notatallshaw notatallshaw left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hope you don't mind, but I've added some more extensive unit tests. Otherwise looks good.

@lets-build-an-ocean

Copy link
Copy Markdown
Author

Hope you don't mind, but I've added some more extensive unit tests. Otherwise looks good.

Thank you for the additional tests and the approval. Much appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

IncompleteRead exception crashes pip instantly, bypassing the built-in download resumption logic

3 participants