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

bpo-43564: preserve original exception in args of FTP URLError #24938

Merged
merged 4 commits into from
Oct 10, 2022
Merged

bpo-43564: preserve original exception in args of FTP URLError #24938

merged 4 commits into from
Oct 10, 2022

Conversation

carljm
Copy link
Member

@carljm carljm commented Mar 19, 2021 •
edited by bedevere-bot
Loading

This makes this case consistent with the other cases in this module where an OSError is caught and wrapped and re-raised as a URLError, in that the original exception is preserved in the args of the URLError for introspection by the caller.

In particular, this means that test.support.transient_internet works with these wrapped errors, so that the FTP urllib2net tests correctly skip instead of failing when the network is not reachable.

https://bugs.python.org/issue43564

@github-actions
Copy link

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Apr 20, 2021
@github-actions github-actions bot removed the stale Stale PR or inactive for long period of time. label Aug 5, 2022
@carljm
Merge branch 'main' into urllibnonet
132c32e 
* main: (5519 commits)
  Minor edits to the Descriptor HowTo Guide (pythonGH-24901)
  Fix link to Lifecycle of a Pull Request in CONTRIBUTING (python#98102)
  pythonGH-94597: deprecate `SafeChildWatcher`, `FastChildWatcher` and `MultiLoopChildWatcher` child watchers  (python#98089)
  Auto-cancel old builds when new commit pushed to branch (python#98009)
  pythongh-95011: Migrate syslog module to Argument Clinic (pythonGH-95012)
  pythongh-68686: Retire eptag ptag scripts (python#98064)
  pythongh-97922: Run the GC only on eval breaker (python#97920)
  GitHub Workflows security hardening (python#96492)
  Add `@ezio-melotti` as codeowner for `.github/`. (python#98079)
  pythongh-97913 Docs: Add walrus operator to the index (python#97921)
  [doc] Fix broken links to C extensions accelerating stdlib modules (python#96914)
  pythongh-97822: Fix http.server documentation reference to test() function (python#98027)
  pythongh-91052: Add PyDict_Unwatch for unwatching a dictionary (python#98055)
  pythonGH-98023: Change default child watcher to PidfdChildWatcher on supported systems (python#98024)
  pythonGH-94182: Run the PidfdChildWatcher on the running loop (python#94184)
  pythongh-92886: make test_ast pass with -O (assertions off) (pythonGH-98058)
  pythongh-92886: make test_coroutines pass with -O (assertions off) (pythonGH-98060)
  pythongh-57179: Add note on symlinks for os.walk (python#94799)
  pythongh-94808: Fix regex on exotic platforms (python#98036)
  pythongh-90085: Remove vestigial -t and -c timeit options (python#94941)
  ...
@carljm
Add NEWS blurb
c188cff
@carljm
Copy link
Member Author

carljm commented Oct 9, 2022

I re-checked this change and I believe it is still correct/useful. The conditions for this to cause a test suite failure are a bit more subtle than described in the issue. If the network is totally unavailable, then the gethostbyname call will fail at

cpython/Lib/urllib/request.py

Line 1558 in aeb28f5

host = socket.gethostbyname(host)
and this error handler already wraps the full network error the way most places in urllib do, which test.support.transient_internet handles correctly.

In order to reproduce this problem, you need a host where there is a network and DNS available (so gethostbyname succeeds) but the network does not have access to the public internet, so the connect_ftp call a bit later at

cpython/Lib/urllib/request.py

Line 1568 in aeb28f5

fw = self.connect_ftp(user, passwd, host, port, dirs, req.timeout)
fails. And it is the error handling around the latter that is inconsistent with the rest of urllib, breaks test.support.transient_internet, and is fixed in this PR.

Without such a host, you can verify this fix by manually adding raise OSError(61, "foo") to the top of the connect_ftp method at

cpython/Lib/urllib/request.py

Line 1588 in aeb28f5

return ftpwrapper(user, passwd, host, port, dirs, timeout,
. Without this fix, ./python.exe -m test test_urllib2net -u network will then fail. With this fix, it will pass and skip the FTP tests due to test.support.transient_internet.

@carljm
Copy link
Member Author

carljm commented Oct 9, 2022

Requesting review from @orsenthil, listed in experts index as expert for urllib.

@carljm carljm requested a review from orsenthil October 9, 2022 10:22
@carljm
Merge branch 'main' into urllibnonet
5d2c846 
* main:
  pythongh-97841: Add methoddef for _filters_mutated (pythongh-98115)
@orsenthil
Copy link
Member

Looks like a right candidate to back port to other supported branches.

important change for CI

@orsenthil orsenthil added needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels Oct 10, 2022
@orsenthil orsenthil merged commit ad817cd into python:main Oct 10, 2022
@miss-islington
Copy link
Contributor

Thanks @carljm for the PR, and @orsenthil for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry @carljm and @orsenthil, I had trouble checking out the 3.11 backport branch.
Please backport using cherry_picker on command line.
cherry_picker ad817cd5c44416da3752ebf9baf16d650703275c 3.11

@miss-islington
Copy link
Contributor

Sorry, @carljm and @orsenthil, I could not cleanly backport this to 3.10 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker ad817cd5c44416da3752ebf9baf16d650703275c 3.10

@AlexWaygood AlexWaygood added needs backport to 3.11 only security fixes and removed needs backport to 3.11 only security fixes labels Oct 10, 2022
@miss-islington
Copy link
Contributor

Thanks @carljm for the PR, and @orsenthil for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

@miss-islington
Copy link
Contributor

Sorry @carljm and @orsenthil, I had trouble checking out the 3.11 backport branch.
Please backport using cherry_picker on command line.
cherry_picker ad817cd5c44416da3752ebf9baf16d650703275c 3.11

@AlexWaygood AlexWaygood removed the needs backport to 3.11 only security fixes label Oct 10, 2022
@AlexWaygood AlexWaygood added the needs backport to 3.11 only security fixes label Oct 10, 2022
@miss-islington
Copy link
Contributor

Thanks @carljm for the PR, and @orsenthil for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Oct 10, 2022
@bedevere-bot
Copy link

GH-98136 is a backport of this pull request to the 3.11 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 10, 2022
@carljm @miss-islington
bpo-43564: preserve original exception in args of FTP URLError (pytho…
b4027ad 
…nGH-24938)

* bpo-43564: preserve original error in args of FTP URLError

* Add NEWS blurb

Co-authored-by: Carl Meyer <carljm@instagram.com>
(cherry picked from commit ad817cd)

Co-authored-by: Carl Meyer <carl@oddbird.net>
miss-islington added a commit that referenced this pull request Oct 10, 2022
@miss-islington @carljm
bpo-43564: preserve original exception in args of FTP URLError (GH-24938
1d05b58 
)

* bpo-43564: preserve original error in args of FTP URLError

* Add NEWS blurb

Co-authored-by: Carl Meyer <carljm@instagram.com>
(cherry picked from commit ad817cd)

Co-authored-by: Carl Meyer <carl@oddbird.net>
@orsenthil orsenthil added needs backport to 3.10 only security fixes and removed needs backport to 3.10 only security fixes labels Oct 10, 2022
@miss-islington
Copy link
Contributor

Thanks @carljm for the PR, and @orsenthil for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry, @carljm and @orsenthil, I could not cleanly backport this to 3.10 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker ad817cd5c44416da3752ebf9baf16d650703275c 3.10

orsenthil pushed a commit that referenced this pull request Oct 10, 2022
@carljm @orsenthil
[3.10] bpo-43564: preserve original exception in args of FTP URLError (…
632310d 
…GH-24938)

* bpo-43564: preserve original error in args of FTP URLError

* Add NEWS blurb

Co-authored-by: Carl Meyer <carljm@instagram.com>.
(cherry picked from commit ad817cd)

Co-authored-by: Carl Meyer <carl@oddbird.net>
@bedevere-bot
Copy link

GH-98138 is a backport of this pull request to the 3.10 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Oct 10, 2022
@carljm carljm mentioned this pull request Apr 10, 2022
Closed
orsenthil added a commit that referenced this pull request Oct 10, 2022
@orsenthil @carljm
[3.10] bpo-43564: preserve original exception in args of FTP URLError (…
1ae7abf 
…GH-24938) (#98138)

* bpo-43564: preserve original error in args of FTP URLError

* Add NEWS blurb

Co-authored-by: Carl Meyer <carljm@instagram.com>.
(cherry picked from commit ad817cd)

Co-authored-by: Carl Meyer <carl@oddbird.net>

Co-authored-by: Carl Meyer <carl@oddbird.net>
@carljm carljm deleted the urllibnonet branch October 10, 2022 06:51
mpage pushed a commit to mpage/cpython that referenced this pull request Oct 11, 2022
@carljm @mpage
bpo-43564: preserve original exception in args of FTP URLError (pytho…
a87db59 
…n#24938)

* bpo-43564: preserve original error in args of FTP URLError

* Add NEWS blurb

Co-authored-by: Carl Meyer <carljm@instagram.com>
@dhomeier dhomeier mentioned this pull request Dec 2, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@orsenthil orsenthil orsenthil approved these changes

Assignees

@orsenthil orsenthil

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@carljm @orsenthil @miss-islington @bedevere-bot @the-knights-who-say-ni @ezio-melotti @AlexWaygood