From e6f05e7e43e0f3e7835140af0f1aba2336b958a9 Mon Sep 17 00:00:00 2001 From: Rob Browning Date: Sat, 25 Sep 2021 19:55:13 -0500 Subject: [PATCH] Remove Client __del__ in favor of context management And drop EPIPE suppression for now. Signed-off-by: Rob Browning Tested-by: Rob Browning --- lib/bup/client.py | 16 ++--- lib/bup/cmd/init.py | 4 +- lib/bup/cmd/save.py | 56 ++++++++-------- lib/bup/cmd/split.py | 51 +++++++------- lib/bup/compat.py | 7 ++ test/int/test_client.py | 144 ++++++++++++++++++++-------------------- 6 files changed, 141 insertions(+), 137 deletions(-) diff --git a/lib/bup/client.py b/lib/bup/client.py index 2532054..178eda5 100644 --- a/lib/bup/client.py +++ b/lib/bup/client.py @@ -3,11 +3,11 @@ from __future__ import print_function from __future__ import absolute_import from binascii import hexlify, unhexlify -import errno, os, re, struct, time, zlib +import os, re, struct, time, zlib import socket from bup import git, ssh, vfs -from bup.compat import environ, range, reraise +from bup.compat import environ, pending_raise, range, reraise from bup.helpers import (Conn, atomically_replaced_file, chunkyreader, debug1, debug2, linereader, lines_until_sentinel, mkdirp, progress, qprogress, DemuxConn) @@ -117,14 +117,12 @@ class Client: self.check_ok() self.sync_indexes() - def __del__(self): - try: + def __enter__(self): + return self + + def __exit__(self, type, value, traceback): + with pending_raise(value, rethrow=False): self.close() - except IOError as e: - if e.errno == errno.EPIPE: - pass - else: - raise def close(self): if self.conn and not self._busy: diff --git a/lib/bup/cmd/init.py b/lib/bup/cmd/init.py index 4be83a2..56fd123 100755 --- a/lib/bup/cmd/init.py +++ b/lib/bup/cmd/init.py @@ -28,5 +28,5 @@ def main(argv): if opt.remote: git.check_repo_or_die() - cli = client.Client(argv_bytes(opt.remote), create=True) - cli.close() + with client.Client(argv_bytes(opt.remote), create=True): + pass diff --git a/lib/bup/cmd/save.py b/lib/bup/cmd/save.py index 0e8ea50..ee78a87 100755 --- a/lib/bup/cmd/save.py +++ b/lib/bup/cmd/save.py @@ -7,7 +7,7 @@ import math, os, stat, sys, time from bup import compat, hashsplit, git, options, index, client, metadata from bup import hlinkdb -from bup.compat import argv_bytes, environ +from bup.compat import argv_bytes, environ, nullcontext from bup.hashsplit import GIT_MODE_TREE, GIT_MODE_FILE, GIT_MODE_SYMLINK from bup.helpers import (add_error, grafted_path_components, handle_ctrl_c, hostname, istty2, log, parse_date_or_fatal, parse_num, @@ -496,43 +496,45 @@ def main(argv): remote_dest = opt.remote or opt.is_reverse if not remote_dest: repo = git - cli = None - w = git.PackWriter(compression_level=opt.compress) + cli = nullcontext() else: try: cli = repo = client.Client(opt.remote) except client.ClientError as e: log('error: %s' % e) sys.exit(1) - w = cli.new_packwriter(compression_level=opt.compress) - sys.stdout.flush() - out = byte_stream(sys.stdout) + # cli creation must be last nontrivial command in each if clause above + with cli: + if not remote_dest: + w = git.PackWriter(compression_level=opt.compress) + else: + w = cli.new_packwriter(compression_level=opt.compress) - if opt.name: - refname = b'refs/heads/%s' % opt.name - parent = repo.read_ref(refname) - else: - refname = parent = None - - tree = save_tree(opt, w) - if opt.tree: - out.write(hexlify(tree)) - out.write(b'\n') - if opt.commit or opt.name: - commit = commit_tree(tree, parent, opt.date, argv, w) - if opt.commit: - out.write(hexlify(commit)) - out.write(b'\n') + sys.stdout.flush() + out = byte_stream(sys.stdout) - w.close() + if opt.name: + refname = b'refs/heads/%s' % opt.name + parent = repo.read_ref(refname) + else: + refname = parent = None - # packwriter must be closed before we can update the ref - if opt.name: - repo.update_ref(refname, commit, parent) + tree = save_tree(opt, w) + if opt.tree: + out.write(hexlify(tree)) + out.write(b'\n') + if opt.commit or opt.name: + commit = commit_tree(tree, parent, opt.date, argv, w) + if opt.commit: + out.write(hexlify(commit)) + out.write(b'\n') + + w.close() - if cli: - cli.close() + # packwriter must be closed before we can update the ref + if opt.name: + repo.update_ref(refname, commit, parent) if saved_errors: log('WARNING: %d errors encountered while saving.\n' % len(saved_errors)) diff --git a/lib/bup/cmd/split.py b/lib/bup/cmd/split.py index 0c1ed4a..1ffe44d 100755 --- a/lib/bup/cmd/split.py +++ b/lib/bup/cmd/split.py @@ -4,7 +4,7 @@ from binascii import hexlify import sys, time from bup import compat, hashsplit, git, options, client -from bup.compat import argv_bytes, environ +from bup.compat import argv_bytes, environ, nullcontext from bup.helpers import (add_error, hostname, log, parse_num, qprogress, reprogress, saved_errors, valid_save_name, @@ -225,42 +225,39 @@ def main(argv): if writing: git.check_repo_or_die() - if not writing: - cli = None - elif remote_dest: + if remote_dest and writing: cli = repo = client.Client(opt.remote) else: - cli = None + cli = nullcontext() repo = git - if opt.name and writing: - refname = opt.name and b'refs/heads/%s' % opt.name - oldref = repo.read_ref(refname) - else: - refname = oldref = None + # cli creation must be last nontrivial command in each if clause above + with cli: + if opt.name and writing: + refname = opt.name and b'refs/heads/%s' % opt.name + oldref = repo.read_ref(refname) + else: + refname = oldref = None - if not writing: - pack_writer = NoOpPackWriter() - elif not remote_dest: - pack_writer = git.PackWriter(compression_level=opt.compress, - max_pack_size=opt.max_pack_size, - max_pack_objects=opt.max_pack_objects) - else: - pack_writer = cli.new_packwriter(compression_level=opt.compress, + if not writing: + pack_writer = NoOpPackWriter() + elif not remote_dest: + pack_writer = git.PackWriter(compression_level=opt.compress, max_pack_size=opt.max_pack_size, max_pack_objects=opt.max_pack_objects) + else: + pack_writer = cli.new_packwriter(compression_level=opt.compress, + max_pack_size=opt.max_pack_size, + max_pack_objects=opt.max_pack_objects) - commit = split(opt, files, oldref, out, pack_writer) - - if pack_writer: - pack_writer.close() + commit = split(opt, files, oldref, out, pack_writer) - # pack_writer must be closed before we can update the ref - if refname: - repo.update_ref(refname, commit, oldref) + if pack_writer: + pack_writer.close() - if cli: - cli.close() + # pack_writer must be closed before we can update the ref + if refname: + repo.update_ref(refname, commit, oldref) secs = time.time() - start_time size = hashsplit.total_split diff --git a/lib/bup/compat.py b/lib/bup/compat.py index 9002025..fbeee4a 100644 --- a/lib/bup/compat.py +++ b/lib/bup/compat.py @@ -13,6 +13,7 @@ py3 = py_maj >= 3 if py3: # pylint: disable=unused-import + from contextlib import nullcontext from os import environb as environ from os import fsdecode, fsencode from shlex import quote @@ -82,6 +83,8 @@ if py3: else: # Python 2 + from contextlib import contextmanager + ModuleNotFoundError = ImportError def fsdecode(x): @@ -97,6 +100,10 @@ else: # Python 2 # pylint: disable=unused-import from bup.py2raise import reraise + @contextmanager + def nullcontext(enter_result=None): + yield enter_result + # on py3 this causes errors, obviously # pylint: disable=undefined-variable input = raw_input diff --git a/test/int/test_client.py b/test/int/test_client.py index b3cdba4..605cffb 100644 --- a/test/int/test_client.py +++ b/test/int/test_client.py @@ -24,16 +24,16 @@ def test_server_split_with_indexes(tmpdir): environ[b'BUP_DIR'] = bupdir = tmpdir git.init_repo(bupdir) lw = git.PackWriter() - c = client.Client(bupdir, create=True) - rw = c.new_packwriter() + with client.Client(bupdir, create=True) as c: + rw = c.new_packwriter() - lw.new_blob(s1) - lw.close() + lw.new_blob(s1) + lw.close() - rw.new_blob(s2) - rw.breakpoint() - rw.new_blob(s1) - rw.close() + rw.new_blob(s2) + rw.breakpoint() + rw.new_blob(s1) + rw.close() def test_multiple_suggestions(tmpdir): @@ -48,52 +48,52 @@ def test_multiple_suggestions(tmpdir): lw.close() assert len(glob.glob(git.repo(b'objects/pack'+IDX_PAT))) == 2 - c = client.Client(bupdir, create=True) - assert len(glob.glob(c.cachedir+IDX_PAT)) == 0 - rw = c.new_packwriter() - s1sha = rw.new_blob(s1) - assert rw.exists(s1sha) - s2sha = rw.new_blob(s2) - - # This is a little hacky, but ensures that we test the - # code under test. First, flush to ensure that we've - # actually sent all the command ('receive-objects-v2') - # and their data to the server. This may be needed if - # the output buffer size is bigger than the data (both - # command and objects) we're writing. To see the need - # for this, change the object sizes at the beginning - # of this file to be very small (e.g. 10 instead of 10k) - c.conn.outp.flush() - - # Then, check if we've already received the idx files. - # This may happen if we're preempted just after writing - # the data, then the server runs and suggests, and only - # then we continue in PackWriter_Remote::_raw_write() - # and check the has_input(), in that case we'll receive - # the idx still in the rw.new_blob() calls above. - # - # In most cases though, that doesn't happen, and we'll - # get past the has_input() check before the server has - # a chance to respond - it has to actually hash the new - # object here, so it takes some time. So also break out - # of the loop if the server has sent something on the - # connection. - # - # Finally, abort this after a little while (about one - # second) just in case something's actually broken. - n = 0 - while (len(glob.glob(c.cachedir+IDX_PAT)) < 2 and - not c.conn.has_input() and n < 10): - time.sleep(0.1) - n += 1 - assert len(glob.glob(c.cachedir+IDX_PAT)) == 2 or c.conn.has_input() - rw.new_blob(s2) - assert rw.objcache.exists(s1sha) - assert rw.objcache.exists(s2sha) - rw.new_blob(s3) - assert len(glob.glob(c.cachedir+IDX_PAT)) == 2 - rw.close() - assert len(glob.glob(c.cachedir+IDX_PAT)) == 3 + with client.Client(bupdir, create=True) as c: + assert len(glob.glob(c.cachedir+IDX_PAT)) == 0 + rw = c.new_packwriter() + s1sha = rw.new_blob(s1) + assert rw.exists(s1sha) + s2sha = rw.new_blob(s2) + + # This is a little hacky, but ensures that we test the + # code under test. First, flush to ensure that we've + # actually sent all the command ('receive-objects-v2') + # and their data to the server. This may be needed if + # the output buffer size is bigger than the data (both + # command and objects) we're writing. To see the need + # for this, change the object sizes at the beginning + # of this file to be very small (e.g. 10 instead of 10k) + c.conn.outp.flush() + + # Then, check if we've already received the idx files. + # This may happen if we're preempted just after writing + # the data, then the server runs and suggests, and only + # then we continue in PackWriter_Remote::_raw_write() + # and check the has_input(), in that case we'll receive + # the idx still in the rw.new_blob() calls above. + # + # In most cases though, that doesn't happen, and we'll + # get past the has_input() check before the server has + # a chance to respond - it has to actually hash the new + # object here, so it takes some time. So also break out + # of the loop if the server has sent something on the + # connection. + # + # Finally, abort this after a little while (about one + # second) just in case something's actually broken. + n = 0 + while (len(glob.glob(c.cachedir+IDX_PAT)) < 2 and + not c.conn.has_input() and n < 10): + time.sleep(0.1) + n += 1 + assert len(glob.glob(c.cachedir+IDX_PAT)) == 2 or c.conn.has_input() + rw.new_blob(s2) + assert rw.objcache.exists(s1sha) + assert rw.objcache.exists(s2sha) + rw.new_blob(s3) + assert len(glob.glob(c.cachedir+IDX_PAT)) == 2 + rw.close() + assert len(glob.glob(c.cachedir+IDX_PAT)) == 3 def test_dumb_client_server(tmpdir): @@ -105,29 +105,29 @@ def test_dumb_client_server(tmpdir): lw.new_blob(s1) lw.close() - c = client.Client(bupdir, create=True) - rw = c.new_packwriter() - assert len(glob.glob(c.cachedir+IDX_PAT)) == 1 - rw.new_blob(s1) - assert len(glob.glob(c.cachedir+IDX_PAT)) == 1 - rw.new_blob(s2) - rw.close() - assert len(glob.glob(c.cachedir+IDX_PAT)) == 2 + with client.Client(bupdir, create=True) as c: + rw = c.new_packwriter() + assert len(glob.glob(c.cachedir+IDX_PAT)) == 1 + rw.new_blob(s1) + assert len(glob.glob(c.cachedir+IDX_PAT)) == 1 + rw.new_blob(s2) + rw.close() + assert len(glob.glob(c.cachedir+IDX_PAT)) == 2 def test_midx_refreshing(tmpdir): environ[b'BUP_DIR'] = bupdir = tmpdir git.init_repo(bupdir) - c = client.Client(bupdir, create=True) - rw = c.new_packwriter() - rw.new_blob(s1) - p1base = rw.breakpoint() - p1name = os.path.join(c.cachedir, p1base) - s1sha = rw.new_blob(s1) # should not be written; it's already in p1 - s2sha = rw.new_blob(s2) - p2base = rw.close() - p2name = os.path.join(c.cachedir, p2base) - del rw + with client.Client(bupdir, create=True) as c: + rw = c.new_packwriter() + rw.new_blob(s1) + p1base = rw.breakpoint() + p1name = os.path.join(c.cachedir, p1base) + s1sha = rw.new_blob(s1) # should not be written; it's already in p1 + s2sha = rw.new_blob(s2) + p2base = rw.close() + p2name = os.path.join(c.cachedir, p2base) + del rw pi = git.PackIdxList(bupdir + b'/objects/pack') assert len(pi.packs) == 2 -- 2.39.2