launchpad-reviewers team mailing list archive

[Colin Watson <mp+433678@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:simple-open-leaks into launchpad:master.

Commit message:
Plug several simple open() resource leaks

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/433678

Brought to you by some initial experiments with https://pypi.org/project/pyastgrep/:

  pyastgrep './/Call/func/Attribute[@attr="read" or @attr="write"]/value/Call/func/Name[@id="open"]'
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:simple-open-leaks into launchpad:master.

diff --git a/lib/lp/archivepublisher/tests/test_publishdistro.py b/lib/lp/archivepublisher/tests/test_publishdistro.py
index 92fdd7a..96ade8e 100644
--- a/lib/lp/archivepublisher/tests/test_publishdistro.py
+++ b/lib/lp/archivepublisher/tests/test_publishdistro.py
@@ -116,7 +116,8 @@ class TestPublishDistro(TestNativePublishingBase):
         self.assertEqual(pub_source.status, PackagePublishingStatus.PUBLISHED)
 
         foo_path = "%s/main/f/foo/foo_666.dsc" % self.pool_dir
-        self.assertEqual(open(foo_path).read().strip(), "foo")
+        with open(foo_path) as foo:
+            self.assertEqual(foo.read().strip(), "foo")
 
     def testDirtySuiteProcessing(self):
         """Test dirty suite processing.
@@ -177,7 +178,8 @@ class TestPublishDistro(TestNativePublishingBase):
         self.assertNotExists(foo_path)
 
         baz_path = "%s/main/b/baz/baz_666.dsc" % self.pool_dir
-        self.assertEqual("baz", open(baz_path).read().strip())
+        with open(baz_path) as baz:
+            self.assertEqual("baz", baz.read().strip())
 
     def publishToArchiveWithOverriddenDistsroot(self, archive):
         """Publish a test package to the specified archive.
@@ -311,14 +313,16 @@ class TestPublishDistro(TestNativePublishingBase):
             "cprov",
             "ppa/ubuntutest/pool/main/b/baz/baz_666.dsc",
         )
-        self.assertEqual("baz", open(baz_path).read().strip())
+        with open(baz_path) as baz:
+            self.assertEqual("baz", baz.read().strip())
 
         bar_path = os.path.join(
             config.personalpackagearchive.root,
             "name16",
             "ppa/ubuntutest/pool/main/b/bar/bar_666.dsc",
         )
-        self.assertEqual("bar", open(bar_path).read().strip())
+        with open(bar_path) as bar:
+            self.assertEqual("bar", bar.read().strip())
 
     @defer.inlineCallbacks
     def testForPrivatePPA(self):
diff --git a/lib/lp/services/librarianserver/tests/test_gc.py b/lib/lp/services/librarianserver/tests/test_gc.py
index 613b53a..e45ad22 100644
--- a/lib/lp/services/librarianserver/tests/test_gc.py
+++ b/lib/lp/services/librarianserver/tests/test_gc.py
@@ -687,10 +687,14 @@ class TestDiskLibrarianGarbageCollection(
             noisefile1_path = os.path.join(noisedir1_path, "abc")
             noisefile2_path = os.path.join(noisedir2_path, "def")
             noisefile3_path = os.path.join(noisedir2_path, "ghi")
-            open(noisefile1_path, "w").write("hello")
-            open(noisefile2_path, "w").write("there")
-            open(noisefile3_path, "w").write("testsuite")
-            open(migrated_path, "w").write("migrant")
+            with open(noisefile1_path, "w") as noisefile1:
+                noisefile1.write("hello")
+            with open(noisefile2_path, "w") as noisefile2:
+                noisefile2.write("there")
+            with open(noisefile3_path, "w") as noisefile3:
+                noisefile3.write("testsuite")
+            with open(migrated_path, "w") as migrated:
+                migrated.write("migrant")
 
             # Pretend it is tomorrow to ensure the files don't count as
             # recently created, and run the delete_unwanted_files process.
diff --git a/lib/lp/soyuz/scripts/tests/test_ppareport.py b/lib/lp/soyuz/scripts/tests/test_ppareport.py
index c8e86b8..92b1bce 100644
--- a/lib/lp/soyuz/scripts/tests/test_ppareport.py
+++ b/lib/lp/soyuz/scripts/tests/test_ppareport.py
@@ -268,15 +268,16 @@ class TestPPAReport(unittest.TestCase):
         os.close(unused_fd)
         reporter = self.getReporter(gen_missing_repos=True, output=output_path)
         reporter.main()
-        self.assertEqual(
-            open(output_path).read().splitlines(),
-            [
-                "= Missing PPA repositories =",
-                "/var/tmp/ppa.test/cprov",
-                "/var/tmp/ppa.test/mark",
-                "",
-            ],
-        )
+        with open(output_path) as output_file:
+            self.assertEqual(
+                output_file.read().splitlines(),
+                [
+                    "= Missing PPA repositories =",
+                    "/var/tmp/ppa.test/cprov",
+                    "/var/tmp/ppa.test/mark",
+                    "",
+                ],
+            )
         # Remove the report file.
         os.remove(output_path)
 
diff --git a/lib/lp/soyuz/tests/test_binarypackagebuildbehaviour.py b/lib/lp/soyuz/tests/test_binarypackagebuildbehaviour.py
index 4d6ca14..8af763b 100644
--- a/lib/lp/soyuz/tests/test_binarypackagebuildbehaviour.py
+++ b/lib/lp/soyuz/tests/test_binarypackagebuildbehaviour.py
@@ -839,7 +839,8 @@ class TestBinaryBuildPackageBehaviourBuildCollection(TestCaseWithFactory):
             # Check that the original file from the worker matches the
             # uncompressed file in the librarian.
             def got_orig_log(ignored):
-                orig_file_content = open(tmp_orig_file_name, "rb").read()
+                with open(tmp_orig_file_name, "rb") as orig_file:
+                    orig_file_content = orig_file.read()
                 self.assertEqual(orig_file_content, uncompressed_file)
 
             d = removeSecurityProxy(worker).getFile(
diff --git a/lib/lp/soyuz/tests/test_publishing.py b/lib/lp/soyuz/tests/test_publishing.py
index 8a84a1a..d2e041c 100644
--- a/lib/lp/soyuz/tests/test_publishing.py
+++ b/lib/lp/soyuz/tests/test_publishing.py
@@ -874,7 +874,8 @@ class TestNativePublishing(TestNativePublishingBase):
         pub_source.publish(self.disk_pool, self.logger)
         self.assertEqual(PackagePublishingStatus.PUBLISHED, pub_source.status)
         pool_path = "%s/main/f/foo/foo_666.dsc" % self.pool_dir
-        self.assertEqual(open(pool_path).read().strip(), "Hello world")
+        with open(pool_path) as pool_file:
+            self.assertEqual(pool_file.read().strip(), "Hello world")
 
     @mock.patch.object(ArtifactoryPool, "addFile")
     def test_publisher_skips_conda_source_packages(self, mock):
@@ -901,7 +902,8 @@ class TestNativePublishing(TestNativePublishingBase):
         pub_binary.publish(self.disk_pool, self.logger)
         self.assertEqual(PackagePublishingStatus.PUBLISHED, pub_binary.status)
         pool_path = "%s/main/f/foo/foo-bin_666_all.deb" % self.pool_dir
-        self.assertEqual(open(pool_path).read().strip(), "Hello world")
+        with open(pool_path) as pool_file:
+            self.assertEqual(pool_file.read().strip(), "Hello world")
 
     def test_publish_isolated_binaries(self):
         # Some binary publications have no associated source publication
@@ -997,7 +999,8 @@ class TestNativePublishing(TestNativePublishingBase):
 
         self.layer.commit()
         self.assertEqual(pub_source.status, PackagePublishingStatus.PENDING)
-        self.assertEqual(open(foo_dsc_path).read().strip(), "Hello world")
+        with open(foo_dsc_path) as foo_dsc:
+            self.assertEqual(foo_dsc.read().strip(), "Hello world")
 
     def testPublishingDifferentContents(self):
         """Test if publishOne refuses to overwrite its own publication."""
@@ -1008,7 +1011,8 @@ class TestNativePublishing(TestNativePublishingBase):
         foo_name = "%s/main/f/foo/foo_666.dsc" % self.pool_dir
         pub_source.sync()
         self.assertEqual(pub_source.status, PackagePublishingStatus.PUBLISHED)
-        self.assertEqual(open(foo_name).read().strip(), "foo is happy")
+        with open(foo_name) as foo:
+            self.assertEqual(foo.read().strip(), "foo is happy")
 
         # try to publish 'foo' again with a different content, it
         # raises internally and keeps the files with the original
@@ -1019,7 +1023,8 @@ class TestNativePublishing(TestNativePublishingBase):
 
         pub_source2.sync()
         self.assertEqual(pub_source2.status, PackagePublishingStatus.PENDING)
-        self.assertEqual(open(foo_name).read().strip(), "foo is happy")
+        with open(foo_name) as foo:
+            self.assertEqual(foo.read().strip(), "foo is happy")
 
     def testPublishingAlreadyInPool(self):
         """Test if publishOne works if file is already in Pool.
@@ -1033,7 +1038,8 @@ class TestNativePublishing(TestNativePublishingBase):
         pub_source.publish(self.disk_pool, self.logger)
         self.layer.commit()
         bar_name = "%s/main/b/bar/bar_666.dsc" % self.pool_dir
-        self.assertEqual(open(bar_name).read().strip(), "bar is good")
+        with open(bar_name) as bar:
+            self.assertEqual(bar.read().strip(), "bar is good")
         pub_source.sync()
         self.assertEqual(pub_source.status, PackagePublishingStatus.PUBLISHED)
 
@@ -1112,7 +1118,8 @@ class TestNativePublishing(TestNativePublishingBase):
             pub_source.sourcepackagerelease.upload_archive, cprov.archive
         )
         foo_name = "%s/main/f/foo/foo_666.dsc" % test_pool_dir
-        self.assertEqual(open(foo_name).read().strip(), "Am I a PPA Record ?")
+        with open(foo_name) as foo:
+            self.assertEqual(foo.read().strip(), "Am I a PPA Record ?")
 
         # Remove locally created dir.
         shutil.rmtree(test_pool_dir)
diff --git a/utilities/roundup-sniffer.py b/utilities/roundup-sniffer.py
index 6e82bd6..3624b13 100755
--- a/utilities/roundup-sniffer.py
+++ b/utilities/roundup-sniffer.py
@@ -65,7 +65,8 @@ class RoundupSniffer:
             urlsafe_b64encode(url.encode("UTF-8")).decode("ASCII"),
         )
         if not exists(filename):
-            open(filename, "wb").write(urlopen(url).read())
+            with open(filename, "wb") as f:
+                f.write(urlopen(url).read())
         return open(filename, "rb")
 
     def get_all_bugs(self):