Restart on filesystem diff #96
Conversation
perfact/zodbsync/commands/watch.py
Outdated
| try: | ||
| self.step() | ||
| except TreeOutdatedException: | ||
| self.release_lock() |
There was a problem hiding this comment.
Why is this needed?
EDIT: OK, I see now that it is needed because step explicitly uses acquire_lock and release_lock, not taking care of releasing it in case of an exception.
Could you please change it such that step either uses with or try ... finally? That function should be responsible for releasing the lock, not run - otherwise each test that calls step manually will need to remember to release the lock. In particular, run is untested (and should probably be marked as such - I was first wondering why this PR reduced the test coverage, but this is the reason) and should therefore contain as few logic as possible (which is why I split everything essential into the step function when I finally managed to add tests for the watcher functionality).
| os.rename(self.base_dir+oldpath, self.base_dir+newpath) | ||
| except OSError as err: | ||
| if err.errno == 2: # no such file or directory | ||
| raise TreeOutdatedException() |
There was a problem hiding this comment.
Please add at least a log line, even if it is not an ERROR level message but, for example, INFO.
| # create subfolder and wait until watch notices change | ||
| with conn.tm: | ||
| app.folder_1.manage_addFolder(id=s_folder_1, title=s_folder_1) | ||
| path = self.repo.path + '/__root__/'+folder_1+'/'+s_folder_1 |
There was a problem hiding this comment.
Interesting that flake8 does not complain about missing spaces around the operators here.
This PR aims to resolve #60 by correctly shutting down the watcher in case the filesystem does not match the currently stored tree! This PR also adds a Test-Case to validate that the correct Exception is thrown in the described use case!