From 30329354b370ed6bfac74290ce0c5d2ab17307d1 Mon Sep 17 00:00:00 2001 From: stuertz Date: Sun, 26 Mar 2017 15:45:10 +0200 Subject: [PATCH 1/7] Skip Test on Windows Currently renaming files is not supported while the the OS doesn't support renaming open files. When closing the file, as done in the code by using http://smmap.readthedocs.io/en/latest/api.html#smmap.mman.StaticWindowMapManager.force_map_handle_removal_win force_map_handle_removal_win, we can rename, but the cache does still have a handle to this file and crashes. --- gitdb/test/db/test_pack.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/gitdb/test/db/test_pack.py b/gitdb/test/db/test_pack.py index a901581..e6c2032 100644 --- a/gitdb/test/db/test_pack.py +++ b/gitdb/test/db/test_pack.py @@ -10,13 +10,17 @@ from gitdb.db import PackedDB from gitdb.exc import BadObject, AmbiguousObjectName +from gitdb.util import mman import os import random +import sys +from unittest import skipIf class TestPackDB(TestDBBase): + @skipIf(sys.platform == "win32", "not supported on windows currently") @with_rw_directory @with_packs_rw def test_writing(self, path): @@ -30,6 +34,11 @@ def test_writing(self, path): # packs removed - rename a file, should affect the glob pack_path = pdb.entities()[0].pack().path() new_pack_path = pack_path + "renamed" + if sys.platform == "win32": + # This is just the beginning: While using thsi function, we are not + # allowed to have any handle to thsi path, which is currently not + # the case. The pack caching does have a handle :-( + mman.force_map_handle_removal_win(pack_path) os.rename(pack_path, new_pack_path) pdb.update_cache(force=True) From 57c3a4fc59b6babe71859b2bf92b2b2fc909ce2a Mon Sep 17 00:00:00 2001 From: stuertz Date: Sun, 26 Mar 2017 15:48:07 +0200 Subject: [PATCH 2/7] Fixed Tests / Code for Windows. Sometimes the OS or some other process has the handle to file a bit longer, and the file could not be deleted immediatly. Retry 10 Times with 100ms distance. --- gitdb/test/performance/test_stream.py | 4 ++-- gitdb/util.py | 27 +++++++++++++++++++++++---- 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/gitdb/test/performance/test_stream.py b/gitdb/test/performance/test_stream.py index 704f4d0..bd8953e 100644 --- a/gitdb/test/performance/test_stream.py +++ b/gitdb/test/performance/test_stream.py @@ -9,7 +9,7 @@ from gitdb.db import LooseObjectDB from gitdb import IStream -from gitdb.util import bin_to_hex +from gitdb.util import bin_to_hex, remove from gitdb.fun import chunk_size from time import time @@ -104,5 +104,5 @@ def test_large_data_streaming(self, path): (size_kib, desc, cs_kib, elapsed_readchunks, size_kib / (elapsed_readchunks or 1)), file=sys.stderr) # del db file so we keep something to do - os.remove(db_file) + remove(db_file) # END for each randomization factor diff --git a/gitdb/util.py b/gitdb/util.py index 242be44..95ab9b2 100644 --- a/gitdb/util.py +++ b/gitdb/util.py @@ -6,6 +6,7 @@ import os import mmap import sys +import time import errno from io import BytesIO @@ -58,7 +59,6 @@ def unpack_from(fmt, data, offset=0): isdir = os.path.isdir isfile = os.path.isfile rename = os.rename -remove = os.remove dirname = os.path.dirname basename = os.path.basename join = os.path.join @@ -67,6 +67,25 @@ def unpack_from(fmt, data, offset=0): close = os.close fsync = os.fsync + +def _retry(func, *args, **kwargs): + # Wrapper around functions, that are problematic on "Windows". Sometimes + # the OS or someone else has still a handle to the file + if sys.platform == "win32": + for _ in xrange(10): + try: + return func(*args, **kwargs) + except Exception: + time.sleep(0.1) + return func(*args, **kwargs) + else: + return func(*args, **kwargs) + + +def remove(*args, **kwargs): + return _retry(os.remove, *args, **kwargs) + + # Backwards compatibility imports from gitdb.const import ( NULL_BIN_SHA, @@ -321,7 +340,7 @@ def open(self, write=False, stream=False): self._fd = os.open(self._filepath, os.O_RDONLY | binary) except: # assure we release our lockfile - os.remove(self._lockfilepath()) + remove(self._lockfilepath()) raise # END handle lockfile # END open descriptor for reading @@ -365,7 +384,7 @@ def _end_writing(self, successful=True): # on windows, rename does not silently overwrite the existing one if sys.platform == "win32": if isfile(self._filepath): - os.remove(self._filepath) + remove(self._filepath) # END remove if exists # END win32 special handling os.rename(lockfile, self._filepath) @@ -376,7 +395,7 @@ def _end_writing(self, successful=True): chmod(self._filepath, int("644", 8)) else: # just delete the file so far, we failed - os.remove(lockfile) + remove(lockfile) # END successful handling #} END utilities From 060e5ea18b010e4d8441a5cfad699aba69593c72 Mon Sep 17 00:00:00 2001 From: stuertz Date: Sun, 26 Mar 2017 22:50:56 +0200 Subject: [PATCH 3/7] Release the file handle, before deleting, otherwise win fails. --- gitdb/test/performance/test_stream.py | 1 + 1 file changed, 1 insertion(+) diff --git a/gitdb/test/performance/test_stream.py b/gitdb/test/performance/test_stream.py index bd8953e..92d28e4 100644 --- a/gitdb/test/performance/test_stream.py +++ b/gitdb/test/performance/test_stream.py @@ -104,5 +104,6 @@ def test_large_data_streaming(self, path): (size_kib, desc, cs_kib, elapsed_readchunks, size_kib / (elapsed_readchunks or 1)), file=sys.stderr) # del db file so we keep something to do + ostream = None # To release the file handle (win) remove(db_file) # END for each randomization factor From d6c097eaf759dabfd3c42b55958962b6e9a05063 Mon Sep 17 00:00:00 2001 From: stuertz Date: Mon, 27 Mar 2017 11:20:48 +0200 Subject: [PATCH 4/7] close smmap handles, to be able to delete files / trees in process (req. for windows) --- gitdb/pack.py | 12 ++++++++++++ gitdb/test/test_pack.py | 7 +++++-- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/gitdb/pack.py b/gitdb/pack.py index 20a4515..115d943 100644 --- a/gitdb/pack.py +++ b/gitdb/pack.py @@ -266,6 +266,10 @@ def __init__(self, indexpath): super(PackIndexFile, self).__init__() self._indexpath = indexpath + def close(self): + mman.force_map_handle_removal_win(self._indexpath) + self._cursor = None + def _set_cache_(self, attr): if attr == "_packfile_checksum": self._packfile_checksum = self._cursor.map()[-40:-20] @@ -527,6 +531,10 @@ class PackFile(LazyMixin): def __init__(self, packpath): self._packpath = packpath + def close(self): + mman.force_map_handle_removal_win(self._packpath) + self._cursor = None + def _set_cache_(self, attr): # we fill the whole cache, whichever attribute gets queried first self._cursor = mman.make_cursor(self._packpath).use_region() @@ -668,6 +676,10 @@ def __init__(self, pack_or_index_path): self._index = self.IndexFileCls("%s.idx" % basename) # PackIndexFile instance self._pack = self.PackFileCls("%s.pack" % basename) # corresponding PackFile instance + def close(self): + self._index.close() + self._pack.close() + def _set_cache_(self, attr): # currently this can only be _offset_map # TODO: make this a simple sorted offset array which can be bisected diff --git a/gitdb/test/test_pack.py b/gitdb/test/test_pack.py index 6e31363..24e2a31 100644 --- a/gitdb/test/test_pack.py +++ b/gitdb/test/test_pack.py @@ -217,10 +217,11 @@ def rewind_streams(): assert os.path.getsize(ppath) > 100 # verify pack - pf = PackFile(ppath) # FIXME: Leaks file-pointer(s)! + pf = PackFile(ppath) assert pf.size() == len(pack_objs) assert pf.version() == PackFile.pack_version_default assert pf.checksum() == pack_sha + pf.close() # verify index if ipath is not None: @@ -231,6 +232,7 @@ def rewind_streams(): assert idx.packfile_checksum() == pack_sha assert idx.indexfile_checksum() == index_sha assert idx.size() == len(pack_objs) + idx.close() # END verify files exist # END for each packpath, indexpath pair @@ -245,7 +247,8 @@ def rewind_streams(): # END for each crc mode # END for each info assert count == len(pack_objs) - + entity.close() + def test_pack_64(self): # TODO: hex-edit a pack helping us to verify that we can handle 64 byte offsets # of course without really needing such a huge pack From 891ac5126540dcb087242f9bb0cadd02ca021324 Mon Sep 17 00:00:00 2001 From: stuertz Date: Mon, 27 Mar 2017 11:36:43 +0200 Subject: [PATCH 5/7] Use range instead of xrange, good enough here --- gitdb/util.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gitdb/util.py b/gitdb/util.py index 95ab9b2..8a1819b 100644 --- a/gitdb/util.py +++ b/gitdb/util.py @@ -72,7 +72,7 @@ def _retry(func, *args, **kwargs): # Wrapper around functions, that are problematic on "Windows". Sometimes # the OS or someone else has still a handle to the file if sys.platform == "win32": - for _ in xrange(10): + for _ in range(10): try: return func(*args, **kwargs) except Exception: From bcdffc46e3993d7743b90982009eec49df667ab4 Mon Sep 17 00:00:00 2001 From: stuertz Date: Mon, 27 Mar 2017 11:44:14 +0200 Subject: [PATCH 6/7] fixed to be py26 compat --- gitdb/test/db/test_pack.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/gitdb/test/db/test_pack.py b/gitdb/test/db/test_pack.py index e6c2032..f6e2751 100644 --- a/gitdb/test/db/test_pack.py +++ b/gitdb/test/db/test_pack.py @@ -16,14 +16,16 @@ import random import sys -from unittest import skipIf +from nose.plugins.skip import SkipTest class TestPackDB(TestDBBase): - @skipIf(sys.platform == "win32", "not supported on windows currently") @with_rw_directory @with_packs_rw def test_writing(self, path): + if sys.platform == "win32": + raise SkipTest("FIXME: Currently fail on windows") + pdb = PackedDB(path) # on demand, we init our pack cache From 7bced788880015075754ce3645cef3a351166ff4 Mon Sep 17 00:00:00 2001 From: stuertz Date: Mon, 27 Mar 2017 11:46:23 +0200 Subject: [PATCH 7/7] Typos --- gitdb/test/db/test_pack.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/gitdb/test/db/test_pack.py b/gitdb/test/db/test_pack.py index f6e2751..9694238 100644 --- a/gitdb/test/db/test_pack.py +++ b/gitdb/test/db/test_pack.py @@ -37,9 +37,9 @@ def test_writing(self, path): pack_path = pdb.entities()[0].pack().path() new_pack_path = pack_path + "renamed" if sys.platform == "win32": - # This is just the beginning: While using thsi function, we are not - # allowed to have any handle to thsi path, which is currently not - # the case. The pack caching does have a handle :-( + # While using this function, we are not allowed to have any handle + # to this path, which is currently not the case. The pack caching + # does still have a handle :-( mman.force_map_handle_removal_win(pack_path) os.rename(pack_path, new_pack_path)