Write/update DSS API definition atomically by calvinnhieu · Pull Request #140 · HumanCellAtlas/dcp-cli

calvinnhieu · 2018-07-23T22:34:02Z

As per @ttung's suggestion (GH-139), these changes update the cached version of the DSS API definition atomically in order to avoid a race condition. This is achieved by writing the fetched file to a temp file and renamed (os.rename) to the desired filename, where the latter operation is atomic.

07/25/18 update:
These changes also handle os.rename Windows specific behavior by deleting the destination file if it exists before renaming. This process is attempted multiple times (5) in order to handle a potential race condition in which the destination file is recreated in between delete and rename operations. These changes also introduce hca.util.fs_helper.py, a static library of functions for interacting with the local filesystem.

Connected to #79

codecov-io · 2018-07-23T22:34:26Z

Codecov Report

Merging #140 into master will increase coverage by 0.68%.
The diff coverage is 94.87%.

@@            Coverage Diff             @@
##           master     #140      +/-   ##
==========================================
+ Coverage   85.75%   86.43%   +0.68%     
==========================================
  Files          31       32       +1     
  Lines        1172     1202      +30     
==========================================
+ Hits         1005     1039      +34     
+ Misses        167      163       -4

Impacted Files	Coverage Δ
hca/util/__init__.py	`93.09% <100%> (+1.98%)`	⬆️
hca/util/fs_helper.py	`94.44% <94.44%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 56e2f09...47a5835. Read the comment docs.

ttung · 2018-07-23T22:36:50Z

    def swagger_spec(self):
        if not self._swagger_spec:
            swagger_url = self.config[self.__class__.__name__].swagger_url
+            temp_swagger_filename = os.path.join(self.config.user_config_dir, "temp_swagger.json")


I would use with tempfile.NamedTemporaryFile

Cool. Thanks!

Oof you should then use it with the option to use the user config dir as the tempdir. Otherwise the rename is not atomic either (since it could actually be a copy).

calvinnhieu · 2018-07-24T16:53:20Z

update: _atomic_write_to_file using NamedTemporaryFile to write temp file and rename (atomic operation) to desired location

ttung

almost there!

ttung · 2018-07-24T18:01:34Z

+        """
+        with tempfile.NamedTemporaryFile('wb', dir=dir, delete=False) as tmp:
+            tmp.write(content)
+            os.rename(tmp.name, filename)


I believe that on Windows, it's not kosher to rename a file that's open. Consider creating the tempfile with delete=False, close it, and then renaming it. Then wrap the entire try-finally with something that cleans up the tempfile in case the rename fails.

Also note from https://docs.python.org/2/library/os.html#os.rename:

On Windows, if dst already exists, OSError will be raised even if it is a file

Here's some possible strategies for defeating that: https://bugs.python.org/issue8828

mweiden

LGTM, once @ttung's last changes have been made.

calvinnhieu · 2018-07-24T21:26:56Z

Addressed Windows issues re: atomic writes

ttung · 2018-07-24T21:32:16Z

+            with tempfile.NamedTemporaryFile('wb', dir=dir, delete=False) as temp:
+                temp_filename = temp.name
+                temp.write(content)
+            while writing and retry_count < max_retries:


suggestion:

for _ in range(max_retries): try: os.rename(temp_filename, dest_filename) break except OSError as ex: if ex.errno == errno.EEXIST: # handle Windows: https://docs.python.org/2/library/os.html#os.rename os.remove(dest_filename) continue raise else: raise Exception("Could not rename {} to {}".format(temp_filename, dest_filename))

you should be able to get rid of writing and retry_count

Right. My Python's a little rusty, thanks!

ttung · 2018-07-24T21:32:25Z

+        :param dir: Shared directory of temp and dest file
+        :param content: Content to write to file
+        """
+        temp_filename = ""


do you actually need this?

Not necessarily, but it clears an IDE warning (and possibly linter?)

ttung · 2018-07-24T21:33:24Z

                assert "swagger" in res.json()
-                with open(swagger_filename, "wb") as fh:
-                    fh.write(res.content)
+                self._atomic_write_to_file(self._get_path_leaf(swagger_filename),


os.path.basename doesn't work here?

Oh, it would - I was covering an edge case that, actually, we don't need to worry about: https://stackoverflow.com/questions/8384737/extract-file-name-from-path-no-matter-what-the-os-path-format
-> Will replace ntpath.basename with os.path.basename

However, _get_path_leaf handles the trailing slash case (e.g. path/to/file/).

Why would swagger_filename end with a trailing slash?

ttung · 2018-07-25T17:44:00Z

            is_cached = os.path.exists(swagger_filename)
-            if (not is_cached) or (is_cached and
-                                   self._get_days_since_last_modified(swagger_filename) >= self._spec_valid_for_days):
+            if (not is_cached) or \


stylistically, i think:

if ((not is_cached) or (is_cached and FSHelper.get_days_since_last_modified(swagger_filename) >= self._spec_valid_for_days)):

is better.

Furthermore, I would say that you can remove the second is_cached as that is implied if you ever reach that clause.

ttung · 2018-07-25T18:28:55Z

                assert "swagger" in res.json()
-                with open(swagger_filename, "wb") as fh:
-                    fh.write(res.content)
+                self._atomic_write_to_file(self._get_path_leaf(swagger_filename),


Why would swagger_filename end with a trailing slash?

ttung · 2018-07-25T18:31:49Z

+                temp.write(content)
+            FSHelper._atomic_rename(temp_filename, dest_filename)
+        finally:
+            if temp_filename and os.path.isfile(temp_filename):


if NamedTemporaryFile throws as exception, then temp_filename will not be defined. one possible strategy is:

if 'temp_filename' in locals() and os.path.isfile(temp_filename):

another strategy is to define temp_filename before the loop.

ttung · 2018-07-25T18:32:20Z

+            FSHelper._atomic_rename(temp_filename, dest_filename)
+        finally:
+            if temp_filename and os.path.isfile(temp_filename):
+                os.remove(temp_filename)


you should wrap os.remove in a try-except EnvironmentError clause.

ttung · 2018-07-25T18:32:55Z

+                os.remove(temp_filename)
+
+    @staticmethod
+    def _atomic_rename(src, dest):


suggestion:

def _atomic_rename(src, dest, max_retries=5):

ttung · 2018-07-25T18:34:10Z

+                FSHelper._handle_rename_error(e, src, dest)
+
+    @staticmethod
+    def _handle_rename_error(err, src, dest):


I think you could just inline this method. It's not particularly large and it's not called elsewhere.

Yea I agree, however, it's pulled out now since when it was inline in _atomic_rename, it violated our linter's Code Complexity limit (7 > 6)

ttung · 2018-07-25T18:34:30Z

+        :param dest: Rename destination filename
+        """
+        if err.errno == errno.EEXIST:
+            os.remove(dest)


os.remove(..) should be wrapped in try-except EnvironmentError

ttung · 2018-07-27T16:43:19Z

            if "swagger_filename" in self.config:
                swagger_filename = self.config.swagger_filename
                if not swagger_filename.startswith("/"):
+                    use_local_config = True


shouldn't use_local_config always be set if swagger_filename is in the config?

furthermore, can you not just use that clause to test rather than creating a new variable?

Right. Good catch

ttung · 2018-07-27T16:51:19Z

-            is_cached = os.path.exists(swagger_filename)
-            if (not is_cached) or (is_cached and
-                                   self._get_days_since_last_modified(swagger_filename) >= self._spec_valid_for_days):
+            if ((not use_local_config) and ((not os.path.exists(swagger_filename)) or


I think this is the wrong set of boolean conditions, if I understand the requirements correctly. If there's a swagger_filename in the config, then we shouldn't ever refresh the config.

ttung · 2018-07-27T16:55:09Z

+        :param shared_dir: Shared directory of temp and dest file
+        :param content: Content to write to file
+        """
+        temp_filename = ""


time_filename = None

ttung · 2018-07-27T17:26:23Z

+                try:
+                    os.remove(temp_filename)
+                except EnvironmentError:
+                    print("Failed to clean up temporary file {}".format(temp_filename))


https://docs.python.org/2/library/warnings.html

ttung · 2018-07-27T17:26:52Z

+                    os.remove(temp_filename)
+                except EnvironmentError:
+                    print("Failed to clean up temporary file {}".format(temp_filename))
+                    raise


I don't think this is a case where we should fatal. I would just warn and move on.

Makes sense

ttung · 2018-07-27T17:27:41Z

+            try:
+                os.remove(dest)
+            except EnvironmentError:
+                print("Failed to delete {}".format(dest))


https://docs.python.org/2/library/warnings.html

ttung · 2018-07-27T17:28:41Z

+            except EnvironmentError:
+                print("Failed to delete {}".format(dest))
+        else:
+            print("Failed to rename {} to {}".format(src, dest))


raise Exception("Failed to rename {} to {}".format(src, dest), err)

ttung · 2018-07-27T17:29:30Z

+        return (now - last_modified).days
+
+    @staticmethod
+    def get_path_leaf(path):


this is still unnecessary as we will never pass in a path with a trailing backslash.

ttung · 2018-07-27T17:31:41Z

 test: lint install
 	# https://github.com/HumanCellAtlas/dcp-cli/issues/127
 	coverage run --source=hca -m unittest discover -v -t . -s test/upload
-	coverage run --source=hca -m unittest discover -v -t . -s test -p test_dss_*.py


can you make this a separate PR and land this ASAP? Seems like a pretty big deal.

ttung · 2018-07-27T21:33:12Z

 	# https://github.com/HumanCellAtlas/dcp-cli/issues/127
 	coverage run --source=hca -m unittest discover -v -t . -s test/upload
 	coverage run --source=hca -m unittest discover -v -t . -s test -p test_dss_*.py
+	coverage run --source=hca -m unittest discover -v -t . -s test -p test_fs_helper.py


remove this change, please.

Oh got it. I'll merge #143 first

ttung · 2018-07-27T21:46:32Z

-            is_cached = os.path.exists(swagger_filename)
-            if (not is_cached) or (is_cached and
-                                   self._get_days_since_last_modified(swagger_filename) >= self._spec_valid_for_days):
+            if (("swagger_filename" not in self.config) and ((not os.path.exists(swagger_filename)) or


do you want the auto-update to happen if someone specifies swagger_filename in the config?

No, if someone specifies swagger_filename in the config, we assume that they are using a local copy of the Swagger file. We wouldn't want to overwrite their local copy and only want to update to the latest Swagger file in the user config directory ~/hca/.config.

I made a mistake reading your code. I think it's correct, but your indentation is confusing. I would do it like this:

if (("swagger_filename" not in self.config) and ((not os.path.exists(swagger_filename)) or (fs.get_days_since_last_modified(swagger_filename) >= self._spec_valid_for_days))):

ttung

see comment about indentation.

ttung · 2018-08-01T21:20:54Z

-            is_cached = os.path.exists(swagger_filename)
-            if (not is_cached) or (is_cached and
-                                   self._get_days_since_last_modified(swagger_filename) >= self._spec_valid_for_days):
+            if (("swagger_filename" not in self.config) and ((not os.path.exists(swagger_filename)) or


I made a mistake reading your code. I think it's correct, but your indentation is confusing. I would do it like this:

if (("swagger_filename" not in self.config) and ((not os.path.exists(swagger_filename)) or (fs.get_days_since_last_modified(swagger_filename) >= self._spec_valid_for_days))):

calvinnhieu self-assigned this Jul 23, 2018

calvinnhieu requested review from kislyuk, mweiden and ttung July 23, 2018 22:34

ttung requested changes Jul 23, 2018

View reviewed changes

calvinnhieu force-pushed the calvinnhieu-atomic-cache-write branch 2 times, most recently from 5370e69 to f331f7b Compare July 24, 2018 16:46

ttung requested changes Jul 24, 2018

View reviewed changes

mweiden reviewed Jul 24, 2018

View reviewed changes

calvinnhieu force-pushed the calvinnhieu-atomic-cache-write branch 2 times, most recently from 6f23f59 to a352bd5 Compare July 24, 2018 21:25

ttung requested changes Jul 24, 2018

View reviewed changes

calvinnhieu force-pushed the calvinnhieu-atomic-cache-write branch 8 times, most recently from c05639c to 02be576 Compare July 25, 2018 06:54

ttung reviewed Jul 25, 2018

View reviewed changes

ttung requested changes Jul 25, 2018

View reviewed changes

calvinnhieu force-pushed the calvinnhieu-atomic-cache-write branch 5 times, most recently from 9a209ff to ef55643 Compare July 26, 2018 20:03

calvinnhieu force-pushed the calvinnhieu-atomic-cache-write branch 2 times, most recently from 298950a to de89aa7 Compare July 26, 2018 22:12

calvinnhieu requested a review from ttung July 26, 2018 22:51

calvinnhieu force-pushed the calvinnhieu-atomic-cache-write branch from de89aa7 to 1870bdc Compare July 26, 2018 23:18

ttung requested changes Jul 27, 2018

View reviewed changes

ttung reviewed Jul 27, 2018

View reviewed changes

calvinnhieu force-pushed the calvinnhieu-atomic-cache-write branch from 1870bdc to da2d24a Compare July 27, 2018 18:51

calvinnhieu requested a review from ttung July 27, 2018 20:09

ttung reviewed Jul 27, 2018

View reviewed changes

calvinnhieu force-pushed the calvinnhieu-atomic-cache-write branch from da2d24a to ea138fd Compare July 30, 2018 18:36

calvinnhieu requested a review from ttung July 30, 2018 18:41

ttung approved these changes Aug 1, 2018

View reviewed changes

Write/update DSS API defn atomically

47a5835

calvinnhieu force-pushed the calvinnhieu-atomic-cache-write branch from ea138fd to 47a5835 Compare August 1, 2018 22:32

calvinnhieu merged commit c68f119 into master Aug 1, 2018

mweiden deleted the calvinnhieu-atomic-cache-write branch August 1, 2018 22:57

Conversation

calvinnhieu commented Jul 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Jul 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

calvinnhieu commented Jul 24, 2018

Uh oh!

ttung left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mweiden left a comment

Choose a reason for hiding this comment

Uh oh!

calvinnhieu commented Jul 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

calvinnhieu Jul 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

calvinnhieu commented Jul 23, 2018 •

edited

Loading

codecov-io commented Jul 23, 2018 •

edited

Loading

calvinnhieu Jul 24, 2018 •

edited

Loading