launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #29198
[Merge] ~cjwatson/launchpad:new-oserror into launchpad:master
Colin Watson has proposed merging ~cjwatson/launchpad:new-oserror into launchpad:master.
Commit message:
Use new OSError exception hierarchy in Python 3
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/429983
This is generally less confusing than the catch-and-maybe-reraise idioms we were previously using.
In one case the new code isn't precisely equivalent to the old code: `lp.services.osutils.two_stage_kill` no longer suppresses `OSError` exceptions other than the ones it explicitly catches (which should only be possible here in the case of programming errors). I think this was probably a mistake in the previous code, since it's exactly the kind of thing that's easy to do by mistake when using catch-and-maybe-reraise idioms.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:new-oserror into launchpad:master.
diff --git a/lib/lp/app/browser/folder.py b/lib/lp/app/browser/folder.py
index ad4c620..ca68a88 100644
--- a/lib/lp/app/browser/folder.py
+++ b/lib/lp/app/browser/folder.py
@@ -6,7 +6,6 @@ __all__ = [
"ExportedImageFolder",
]
-import errno
import os
import re
import time
@@ -86,14 +85,8 @@ class ExportedFolder:
name = os.path.basename(filename)
try:
fileobj = File(filename, name)
- except OSError as ioerror:
- expected = (errno.ENOENT, errno.EISDIR, errno.ENOTDIR)
- if ioerror.errno in expected:
- # No such file or is a directory.
- raise NotFound(self, name)
- else:
- # Some other IOError that we're not expecting.
- raise
+ except (FileNotFoundError, IsADirectoryError, NotADirectoryError):
+ raise NotFound(self, name)
# TODO: Set an appropriate charset too. There may be zope code we
# can reuse for this.
diff --git a/lib/lp/archivepublisher/publishing.py b/lib/lp/archivepublisher/publishing.py
index 87a78b9..f714c77 100644
--- a/lib/lp/archivepublisher/publishing.py
+++ b/lib/lp/archivepublisher/publishing.py
@@ -11,7 +11,6 @@ __all__ = [
]
import bz2
-import errno
import gzip
import hashlib
import lzma
@@ -1377,9 +1376,8 @@ class Publisher:
)
extra_files.add(remove_suffix(dep11_path))
extra_files.add(dep11_path)
- except OSError as e:
- if e.errno != errno.ENOENT:
- raise
+ except FileNotFoundError:
+ pass
cnf_dir = os.path.join(suite_dir, component, "cnf")
try:
for cnf_file in os.listdir(cnf_dir):
@@ -1387,9 +1385,8 @@ class Publisher:
cnf_path = os.path.join(component, "cnf", cnf_file)
extra_files.add(remove_suffix(cnf_path))
extra_files.add(cnf_path)
- except OSError as e:
- if e.errno != errno.ENOENT:
- raise
+ except FileNotFoundError:
+ pass
for architecture in all_architectures:
for contents_path in get_suffixed_indices(
"Contents-" + architecture
@@ -1577,9 +1574,8 @@ class Publisher:
continue
i18n_files.add(remove_suffix(entry.name))
i18n_files.add(entry.name)
- except OSError as e:
- if e.errno != errno.ENOENT:
- raise
+ except FileNotFoundError:
+ pass
if not i18n_files:
# If the i18n directory doesn't exist or is empty, we don't need
# to index it.
diff --git a/lib/lp/archiveuploader/dscfile.py b/lib/lp/archiveuploader/dscfile.py
index d16de4c..c2e7941 100644
--- a/lib/lp/archiveuploader/dscfile.py
+++ b/lib/lp/archiveuploader/dscfile.py
@@ -16,7 +16,6 @@ __all__ = [
"SignableTagFile",
]
-import errno
import glob
import io
import os
@@ -95,17 +94,15 @@ def cleanup_unpacked_dir(unpacked_dir):
"""
try:
shutil.rmtree(unpacked_dir)
+ except PermissionError:
+ result = os.system("chmod -R u+rwx " + unpacked_dir)
+ if result != 0:
+ raise UploadError("chmod failed with %s" % result)
+ shutil.rmtree(unpacked_dir)
except OSError as error:
- if errno.errorcode[error.errno] != "EACCES":
- raise UploadError(
- "couldn't remove tmp dir %s: code %s"
- % (unpacked_dir, error.errno)
- )
- else:
- result = os.system("chmod -R u+rwx " + unpacked_dir)
- if result != 0:
- raise UploadError("chmod failed with %s" % result)
- shutil.rmtree(unpacked_dir)
+ raise UploadError(
+ "couldn't remove tmp dir %s: code %s" % (unpacked_dir, error.errno)
+ )
class SignableTagFile:
diff --git a/lib/lp/bugs/externalbugtracker/tests/test_debbugs.py b/lib/lp/bugs/externalbugtracker/tests/test_debbugs.py
index f019c72..dd42606 100644
--- a/lib/lp/bugs/externalbugtracker/tests/test_debbugs.py
+++ b/lib/lp/bugs/externalbugtracker/tests/test_debbugs.py
@@ -4,7 +4,6 @@
"""Tests for the Debian bugtracker"""
import email.message
-import errno
import os
from testtools.matchers import Equals, IsInstance, raises
@@ -30,9 +29,8 @@ class TestDebBugs(TestCase):
bucket = os.path.join(self.tempdir, "db-h", bucket_name)
try:
os.makedirs(bucket)
- except OSError as e:
- if e.errno != errno.EEXIST:
- raise
+ except FileExistsError:
+ pass
m = email.message.Message()
# For whatever reason Date is a unix timestamp not an email datestamp
m["Date"] = "1000000000"
diff --git a/lib/lp/bugs/scripts/debbugs.py b/lib/lp/bugs/scripts/debbugs.py
index fc85e34..a0307fc 100644
--- a/lib/lp/bugs/scripts/debbugs.py
+++ b/lib/lp/bugs/scripts/debbugs.py
@@ -177,10 +177,8 @@ class Database:
try:
fd = open(summary)
- except OSError as e:
- if e.errno == 2:
- raise SummaryMissing(summary)
- raise
+ except FileNotFoundError:
+ raise SummaryMissing(summary)
try:
message = email.message_from_file(fd)
@@ -222,10 +220,8 @@ class Database:
try:
fd = open(report, "rb")
- except OSError as e:
- if e.errno == 2:
- raise ReportMissing(report)
- raise
+ except FileNotFoundError:
+ raise ReportMissing(report)
bug.report = fd.read()
fd.close()
@@ -278,10 +274,8 @@ class Database:
if process.returncode != 0:
raise LogParseFailed(errors)
- except OSError as e:
- if e.errno == 2:
- raise LogMissing(log)
- raise
+ except FileNotFoundError:
+ raise LogMissing(log)
bug.comments = comments
diff --git a/lib/lp/codehosting/sftp.py b/lib/lp/codehosting/sftp.py
index 728bb3e..fc2d4e8 100644
--- a/lib/lp/codehosting/sftp.py
+++ b/lib/lp/codehosting/sftp.py
@@ -18,7 +18,6 @@ __all__ = [
]
-import errno
import os
import stat
from copy import copy
@@ -58,9 +57,7 @@ class FatLocalTransport(LocalTransport):
osutils.check_legal_path(abspath)
try:
chunk_file = os.open(abspath, os.O_CREAT | os.O_WRONLY)
- except OSError as e:
- if e.errno != errno.EISDIR:
- raise
+ except IsADirectoryError:
raise FileIsADirectory(name)
os.lseek(chunk_file, offset, 0)
os.write(chunk_file, data)
diff --git a/lib/lp/registry/tests/test_mlists.py b/lib/lp/registry/tests/test_mlists.py
index ad68186..42fef42 100644
--- a/lib/lp/registry/tests/test_mlists.py
+++ b/lib/lp/registry/tests/test_mlists.py
@@ -3,7 +3,6 @@
"""Test mailing list stuff."""
-import errno
import os
import tempfile
import unittest
@@ -58,9 +57,8 @@ class BaseMailingListImportTest(unittest.TestCase):
def tearDown(self):
try:
os.remove(self.filename)
- except OSError as error:
- if error.errno != errno.ENOENT:
- raise
+ except FileNotFoundError:
+ pass
def _makeList(self, name, owner):
self.team, self.mailing_list = factory.makeTeamAndMailingList(
diff --git a/lib/lp/services/command_spawner.py b/lib/lp/services/command_spawner.py
index 8bf3ccc..fd3a74a 100644
--- a/lib/lp/services/command_spawner.py
+++ b/lib/lp/services/command_spawner.py
@@ -9,7 +9,6 @@ __all__ = [
"ReturnCodeReceiver",
]
-import errno
import select
import subprocess
from fcntl import F_GETFL, F_SETFL, fcntl
@@ -175,11 +174,9 @@ class CommandSpawner:
"""Read output from `pipe_file`."""
try:
output = pipe_file.read()
- except OSError as e:
- # "Resource temporarily unavailable"--not an error really,
- # just means there's nothing to read.
- if e.errno != errno.EAGAIN:
- raise
+ except BlockingIOError:
+ # This just means there's nothing to read.
+ pass
else:
if len(output) > 0:
self._handle(process, event, output)
diff --git a/lib/lp/services/doc/pidfile.rst b/lib/lp/services/doc/pidfile.rst
index 77e8000..70eabb3 100644
--- a/lib/lp/services/doc/pidfile.rst
+++ b/lib/lp/services/doc/pidfile.rst
@@ -80,7 +80,6 @@ The pidfile must also be removed if the process is exited with SIGINT (Ctrl-C)
or SIGTERM, too. We'll demonstrate this with a couple of functions, because
we'll need them again later.
- >>> import errno
>>> import time
>>> subprocess_code = '''
... import time
@@ -111,11 +110,8 @@ we'll need them again later.
... try:
... # Is it still here at all?
... os.kill(pid, 0)
- ... except OSError as e:
- ... if e.errno == errno.ESRCH:
- ... print("Error: pid file was not removed")
- ... else:
- ... raise
+ ... except ProcessLookupError:
+ ... print("Error: pid file was not removed")
... else:
... print("Error: process did not exit")
...
diff --git a/lib/lp/services/librarianserver/librariangc.py b/lib/lp/services/librarianserver/librariangc.py
index 0ab69ad..a0ec1ad 100644
--- a/lib/lp/services/librarianserver/librariangc.py
+++ b/lib/lp/services/librarianserver/librariangc.py
@@ -3,7 +3,6 @@
"""Librarian garbage collection routines"""
-import errno
import hashlib
import multiprocessing.pool
import os
@@ -589,9 +588,8 @@ class UnreferencedContentPruner:
try:
os.unlink(path)
removed.append("filesystem")
- except OSError as e:
- if e.errno != errno.ENOENT:
- raise
+ except FileNotFoundError:
+ pass
# Remove the file from Swift, if it hasn't already been.
if self.swift_enabled:
diff --git a/lib/lp/services/librarianserver/storage.py b/lib/lp/services/librarianserver/storage.py
index 0117f2e..2c2437b 100644
--- a/lib/lp/services/librarianserver/storage.py
+++ b/lib/lp/services/librarianserver/storage.py
@@ -1,7 +1,6 @@
# Copyright 2009-2021 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
-import errno
import hashlib
import os
import shutil
@@ -56,9 +55,8 @@ def makedirs_fsync(name, mode=0o777):
if head and tail and not os.path.exists(head):
try:
makedirs_fsync(head, mode)
- except OSError as e:
- if e.errno != errno.EEXIST:
- raise
+ except FileExistsError:
+ pass
if tail == os.curdir:
return
os.mkdir(name, mode)
@@ -100,9 +98,8 @@ class LibrarianStorage:
self.incoming = os.path.join(self.directory, "incoming")
try:
os.mkdir(self.incoming)
- except OSError as e:
- if e.errno != errno.EEXIST:
- raise
+ except FileExistsError:
+ pass
def hasFile(self, fileid):
return os.access(self._fileLocation(fileid), os.F_OK)
@@ -321,10 +318,9 @@ class LibraryFileUpload:
raise DuplicateFileIDError(fileID)
try:
makedirs_fsync(os.path.dirname(location))
- except OSError as e:
+ except FileExistsError:
# If the directory already exists, that's ok.
- if e.errno != errno.EEXIST:
- raise
+ pass
shutil.move(self.tmpfilepath, location)
fsync_path(location)
fsync_path(os.path.dirname(location), dir=True)
diff --git a/lib/lp/services/osutils.py b/lib/lp/services/osutils.py
index 911e173..1a2e78a 100644
--- a/lib/lp/services/osutils.py
+++ b/lib/lp/services/osutils.py
@@ -16,7 +16,6 @@ __all__ = [
"write_file",
]
-import errno
import os.path
import shutil
import time
@@ -67,10 +66,8 @@ def ensure_directory_exists(directory, mode=0o777):
"""
try:
os.makedirs(directory, mode=mode)
- except OSError as e:
- if e.errno == errno.EEXIST:
- return False
- raise
+ except FileExistsError:
+ return False
return True
@@ -87,23 +84,18 @@ def open_for_writing(filename, mode, dirmode=0o777):
"""
try:
return open(filename, mode)
- except OSError as e:
- if e.errno == errno.ENOENT:
- os.makedirs(os.path.dirname(filename), mode=dirmode)
- return open(filename, mode)
- raise
+ except FileNotFoundError:
+ os.makedirs(os.path.dirname(filename), mode=dirmode)
+ return open(filename, mode)
def _kill_may_race(pid, signal_number):
"""Kill a pid accepting that it may not exist."""
try:
os.kill(pid, signal_number)
- except OSError as e:
- if e.errno in (errno.ESRCH, errno.ECHILD):
- # Process has already been killed.
- return
- # Some other issue (e.g. different user owns it)
- raise
+ except (ProcessLookupError, ChildProcessError):
+ # Process has already been killed.
+ return
def two_stage_kill(pid, poll_interval=0.1, num_polls=50, get_status=True):
@@ -133,11 +125,10 @@ def two_stage_kill(pid, poll_interval=0.1, num_polls=50, get_status=True):
if not process_exists(pid):
return
time.sleep(poll_interval)
- except OSError as e:
- if e.errno in (errno.ESRCH, errno.ECHILD):
- # Raised if the process is gone by the time we try to get the
- # return value.
- return
+ except (ProcessLookupError, ChildProcessError):
+ # Raised if the process is gone by the time we try to get the
+ # return value.
+ return
# The process is still around, so terminate it violently.
_kill_may_race(pid, SIGKILL)
@@ -176,9 +167,8 @@ def remove_if_exists(path):
"""Remove the given file if it exists."""
try:
os.remove(path)
- except OSError as e:
- if e.errno != errno.ENOENT:
- raise
+ except FileNotFoundError:
+ pass
def write_file(path, content):
@@ -190,11 +180,7 @@ def process_exists(pid):
"""Return True if the specified process already exists."""
try:
os.kill(pid, 0)
- except os.error as err:
- if err.errno == errno.ESRCH:
- # All is well - the process doesn't exist.
- return False
- else:
- # We got a strange OSError, which we'll pass upwards.
- raise
+ except ProcessLookupError:
+ # All is well - the process doesn't exist.
+ return False
return True
diff --git a/lib/lp/services/sitesearch/tests/bingserviceharness.py b/lib/lp/services/sitesearch/tests/bingserviceharness.py
index 68cb697..4e3fe98 100644
--- a/lib/lp/services/sitesearch/tests/bingserviceharness.py
+++ b/lib/lp/services/sitesearch/tests/bingserviceharness.py
@@ -7,8 +7,6 @@ Fixtures for running the Bing test webservice.
__all__ = ["BingServiceTestSetup"]
-
-import errno
import os
import signal
@@ -95,11 +93,10 @@ class BingServiceTestSetup:
if cls.service:
try:
os.kill(cls.service.pid, signal.SIGTERM)
- except OSError as error:
- if error.errno != errno.ESRCH:
- raise
+ except ProcessLookupError:
# The process with the given pid doesn't exist, so there's
# nothing to kill or wait for.
+ pass
else:
cls.service.wait()
cls.service = None
diff --git a/lib/lp/services/sitesearch/testservice.py b/lib/lp/services/sitesearch/testservice.py
index 7384b03..c602f5d 100644
--- a/lib/lp/services/sitesearch/testservice.py
+++ b/lib/lp/services/sitesearch/testservice.py
@@ -8,8 +8,6 @@ This script runs a simple HTTP server. The server returns files
when given certain user-configurable URLs.
"""
-
-import errno
import os
import signal
import socket
@@ -111,13 +109,10 @@ def wait_for_service(host, port, timeout=15.0):
while True:
try:
sock.connect((host, port))
- except OSError as err:
- if err.args[0] in [errno.ECONNREFUSED, errno.ECONNABORTED]:
- elapsed = time.time() - start
- if elapsed > timeout:
- raise RuntimeError("Socket poll time exceeded.")
- else:
- raise
+ except (ConnectionRefusedError, ConnectionAbortedError):
+ elapsed = time.time() - start
+ if elapsed > timeout:
+ raise RuntimeError("Socket poll time exceeded.")
else:
break
time.sleep(0.1)
@@ -143,12 +138,9 @@ def wait_for_service_shutdown(host, port, seconds_to_wait=10.0):
try:
sock.connect((host, port))
sock.close()
- except OSError as err:
- if err.args[0] == errno.ECONNREFUSED:
- # Success! The socket is closed.
- return
- else:
- raise
+ except ConnectionRefusedError:
+ # Success! The socket is closed.
+ return
else:
elapsed = time.time() - start
if elapsed > seconds_to_wait:
@@ -215,12 +207,8 @@ def kill_running_process(service_name, host, port):
# between freeing the socket in the killed process, and
# opening it in the current one.
wait_for_service_shutdown(host, port)
- except os.error as err:
- if err.errno == errno.ESRCH:
- # Whoops, we got a 'No such process' error. The PID file
- # is probably stale, so we'll remove it to prevent trash
- # from lying around in the test environment.
- # See bug #237086.
- remove_if_exists(pidfile_path(service_name))
- else:
- raise
+ except ProcessLookupError:
+ # The PID file is probably stale, so we'll remove it to
+ # prevent trash from lying around in the test environment.
+ # See bug #237086.
+ remove_if_exists(pidfile_path(service_name))
diff --git a/lib/lp/services/tests/test_osutils.py b/lib/lp/services/tests/test_osutils.py
index d344dfe..c576c21 100644
--- a/lib/lp/services/tests/test_osutils.py
+++ b/lib/lp/services/tests/test_osutils.py
@@ -118,9 +118,7 @@ class TestProcessExists(TestCase):
self.assertTrue(process_exists(pid))
def test_with_process_not_running(self):
- exception = OSError()
- exception.errno = errno.ESRCH
- self.useFixture(MockPatch("os.kill", side_effect=exception))
+ self.useFixture(MockPatch("os.kill", side_effect=ProcessLookupError))
self.assertFalse(process_exists(123))
def test_with_unknown_error(self):
diff --git a/lib/lp/soyuz/tests/test_packagediff.py b/lib/lp/soyuz/tests/test_packagediff.py
index 4c0c48b..d87ddaf 100644
--- a/lib/lp/soyuz/tests/test_packagediff.py
+++ b/lib/lp/soyuz/tests/test_packagediff.py
@@ -3,7 +3,6 @@
"""Test source package diffs."""
-import errno
import os.path
from datetime import datetime
from textwrap import dedent
@@ -202,8 +201,7 @@ class TestPackageDiffs(TestCaseWithFactory):
with open(marker_path) as marker:
debdiff_pid = int(marker.readline())
debdiff_tmpdir = marker.readline().rstrip("\n")
- err = self.assertRaises(OSError, os.kill, debdiff_pid, 0)
- self.assertEqual(errno.ESRCH, err.errno)
+ self.assertRaises(ProcessLookupError, os.kill, debdiff_pid, 0)
self.assertFalse(os.path.exists(debdiff_tmpdir))
def test_packagediff_max_size(self):
@@ -235,8 +233,7 @@ class TestPackageDiffs(TestCaseWithFactory):
with open(marker_path) as marker:
debdiff_pid = int(marker.readline())
debdiff_tmpdir = marker.readline().rstrip("\n")
- err = self.assertRaises(OSError, os.kill, debdiff_pid, 0)
- self.assertEqual(errno.ESRCH, err.errno)
+ self.assertRaises(ProcessLookupError, os.kill, debdiff_pid, 0)
self.assertFalse(os.path.exists(debdiff_tmpdir))
def test_packagediff_blacklist(self):
diff --git a/lib/lp/testing/layers.py b/lib/lp/testing/layers.py
index 0777457..a6fae41 100644
--- a/lib/lp/testing/layers.py
+++ b/lib/lp/testing/layers.py
@@ -47,7 +47,6 @@ __all__ = [
import base64
import datetime
-import errno
import gc
import os
import select
@@ -1687,15 +1686,11 @@ class LayerProcessController:
"""
try:
os.kill(cls.appserver.pid, sig)
- except OSError as error:
- if error.errno == errno.ESRCH:
- # The child process doesn't exist. Maybe it went away by the
- # time we got here.
- cls.appserver = None
- return False
- else:
- # Something else went wrong.
- raise
+ except ProcessLookupError:
+ # The child process doesn't exist. Maybe it went away by the
+ # time we got here.
+ cls.appserver = None
+ return False
else:
return True
@@ -1759,9 +1754,8 @@ class LayerProcessController:
# Don't worry if the process no longer exists.
try:
os.kill(pid, signal.SIGTERM)
- except OSError as error:
- if error.errno != errno.ESRCH:
- raise
+ except ProcessLookupError:
+ pass
pidfile.remove_pidfile("launchpad", cls.appserver_config)
@classmethod
@@ -1801,10 +1795,8 @@ class LayerProcessController:
% error.code
)
except URLError as error:
- # We are interested in a wrapped socket.error.
- if not isinstance(error.reason, socket.error):
- raise
- if error.reason.args[0] != errno.ECONNREFUSED:
+ # We are interested in a wrapped ConnectionRefusedError.
+ if not isinstance(error.reason, ConnectionRefusedError):
raise
returncode = cls.appserver.poll()
if returncode is not None:
diff --git a/lib/lp/translations/pottery/tests/test_detect_intltool.py b/lib/lp/translations/pottery/tests/test_detect_intltool.py
index cbf90d2..c25078f 100644
--- a/lib/lp/translations/pottery/tests/test_detect_intltool.py
+++ b/lib/lp/translations/pottery/tests/test_detect_intltool.py
@@ -1,7 +1,6 @@
# Copyright 2009-2017 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
-import errno
import os
import tarfile
from textwrap import dedent
@@ -48,10 +47,9 @@ class SetupTestPackageMixin:
if directory != "":
try:
os.makedirs(directory)
- except OSError as e:
+ except FileExistsError:
# Doesn't matter if it already exists.
- if e.errno != errno.EEXIST:
- raise
+ pass
with open(path, "w") as the_file:
the_file.write(content)
diff --git a/test_on_merge.py b/test_on_merge.py
index 63087f0..99f343d 100755
--- a/test_on_merge.py
+++ b/test_on_merge.py
@@ -7,7 +7,6 @@
import _pythonpath # noqa: F401
-import errno
import os
import select
import sys
@@ -241,12 +240,9 @@ def nice_killpg(pgid):
# Give the processes some time to shut down.
time.sleep(3)
- except OSError as exc:
- if exc.errno == errno.ESRCH:
- # We tried to call os.killpg() and found the group to be empty.
- pass
- else:
- raise
+ except ProcessLookupError:
+ # We tried to call os.killpg() and found the group to be empty.
+ pass
print("Process group %d is now empty." % pgid)
