Change the job.postprocess_job_state to catch OSError and not IOError, since OSError is what os.remove throws when the file does not exist.
It also cleans up some more of the backing file locking for job_state, so that the read and write code in server_job is compliant as well. The main difference is that while read_from_file does not provide generally safe locking and synching behaviour, it does do so for its final synchronization. This makes it easier for non-job_state code to be safe, since it only needs to lock the file it passes in. It also moves the locking decorator out of the class, to make it easier to use on the fly. It also separates out the with-lock and with-sync code. Signed-off-by: John Admanski <[email protected]> --- autotest/client/common_lib/base_job.py 2010-01-21 11:33:17.000000000 -0800 +++ autotest/client/common_lib/base_job.py 2010-01-21 15:14:24.000000000 -0800 @@ -117,6 +117,51 @@ return dir_property +# decorator for use with job_state methods +def with_backing_lock(method): + """A decorator to perform a lock-*-unlock cycle. + + When applied to a method, this decorator will automatically wrap + calls to the method in a backing file lock and before the call + followed by a backing file unlock. + """ + def wrapped_method(self, *args, **dargs): + already_have_lock = self._backing_file_lock is not None + if not already_have_lock: + self._lock_backing_file() + try: + return method(self, *args, **dargs) + finally: + if not already_have_lock: + self._unlock_backing_file() + wrapped_method.__name__ = method.__name__ + wrapped_method.__doc__ = method.__doc__ + return wrapped_method + + +# decorator for use with job_state methods +def with_backing_file(method): + """A decorator to perform a lock-read-*-write-unlock cycle. + + When applied to a method, this decorator will automatically wrap + calls to the method in a lock-and-read before the call followed by a + write-and-unlock. Any operation that is reading or writing state + should be decorated with this method to ensure that backing file + state is consistently maintained. + """ + @with_backing_lock + def wrapped_method(self, *args, **dargs): + self._read_from_backing_file() + try: + return method(self, *args, **dargs) + finally: + self._write_to_backing_file() + wrapped_method.__name__ = method.__name__ + wrapped_method.__doc__ = method.__doc__ + return wrapped_method + + + class job_state(object): """A class for managing explicit job and user state, optionally persistent. @@ -140,33 +185,6 @@ self._backing_file_lock = None - def _with_backing_file(method): - """A decorator to perform a lock-read-*-write-unlock cycle. - - When applied to a method, this decorator will automatically wrap - calls to the method in a lock-and-read before the call followed by a - write-and-unlock. Any operation that is reading or writing state - should be decorated with this method to ensure that backing file - state is consistently maintained. - """ - def wrapped_method(self, *args, **dargs): - already_have_lock = self._backing_file_lock is not None - if not already_have_lock: - self._lock_backing_file() - try: - self._read_from_backing_file() - try: - return method(self, *args, **dargs) - finally: - self._write_to_backing_file() - finally: - if not already_have_lock: - self._unlock_backing_file() - wrapped_method.__name__ = method.__name__ - wrapped_method.__doc__ = method.__doc__ - return wrapped_method - - def _lock_backing_file(self): """Acquire a lock on the backing file.""" if self._backing_file: @@ -229,7 +247,8 @@ logging.debug('Replacing in-memory state with on-disk state ' 'from %s', file_path) - self._write_to_backing_file() + # lock the backing file before we refresh it + with_backing_lock(self.__class__._write_to_backing_file)(self) def write_to_file(self, file_path): @@ -239,8 +258,7 @@ Must be writable. @warning This method is intentionally concurrency-unsafe. It makes no - attempt to control concurrent access to the file at file_path, or - to the backing file if one exists. + attempt to control concurrent access to the file at file_path. """ outfile = open(file_path, 'w') try: @@ -269,7 +287,7 @@ self.write_to_file(self._backing_file) - @_with_backing_file + @with_backing_file def _synchronize_backing_file(self): """Synchronizes the contents of the in-memory and on-disk state.""" # state is implicitly synchronized in _with_backing_file methods @@ -294,7 +312,7 @@ self._synchronize_backing_file() - @_with_backing_file + @with_backing_file def get(self, namespace, name, default=NO_DEFAULT): """Returns the value associated with a particular name. @@ -317,7 +335,7 @@ return default - @_with_backing_file + @with_backing_file def set(self, namespace, name, value): """Saves the value given with the provided name. @@ -331,7 +349,7 @@ name, value) - @_with_backing_file + @with_backing_file def has(self, namespace, name): """Return a boolean indicating if namespace.name is defined. @@ -344,7 +362,7 @@ return namespace in self._state and name in self._state[namespace] - @_with_backing_file + @with_backing_file def discard(self, namespace, name): """If namespace.name is a defined value, deletes it. @@ -362,7 +380,7 @@ namespace, name) - @_with_backing_file + @with_backing_file def discard_namespace(self, namespace): """Delete all defined namespace.* names. --- autotest/client/common_lib/base_job_unittest.py 2010-01-21 11:33:15.000000000 -0800 +++ autotest/client/common_lib/base_job_unittest.py 2010-01-21 15:14:24.000000000 -0800 @@ -733,12 +733,12 @@ ut_self = self class mocked_job_state(base_job.job_state): def read_from_file(self, file_path, merge=True): - if self._backing_file: + if self._backing_file and file_path == self._backing_file: ut_self.assertNotEqual(None, self._backing_file_lock) return super(mocked_job_state, self).read_from_file( file_path, merge=True) def write_to_file(self, file_path): - if self._backing_file: + if self._backing_file and file_path == self._backing_file: ut_self.assertNotEqual(None, self._backing_file_lock) return super(mocked_job_state, self).write_to_file(file_path) self.state = mocked_job_state() @@ -803,6 +803,17 @@ self.state.set_backing_file('another_backing_file') + def test_read_from_a_non_backing_file(self): + state = base_job.job_state() + state.set('ns9', 'var9', 9999) + state.write_to_file('non_backing_file') + self.state.read_from_file('non_backing_file') + + + def test_write_to_a_non_backing_file(self): + self.state.write_to_file('non_backing_file') + + class test_job_state_property_factory(unittest.TestCase): def setUp(self): class job_stub(object): --- autotest/server/server_job.py 2010-01-21 11:33:22.000000000 -0800 +++ autotest/server/server_job.py 2010-01-21 15:14:24.000000000 -0800 @@ -1100,6 +1100,8 @@ # dump the state out to a tempfile fd, file_path = tempfile.mkstemp(dir=self.tmpdir) os.close(fd) + + # write_to_file doesn't need locking, we exclusively own file_path self._state.write_to_file(file_path) return file_path @@ -1119,7 +1121,7 @@ self._state.read_from_file(state_path) try: os.remove(state_path) - except IOError, e: + except OSError, e: # ignore file-not-found errors if e.errno != errno.ENOENT: raise _______________________________________________ Autotest mailing list [email protected] http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
