Added a global lock for the safe_save function to make sure XML files…

[drmarkwslater]

… don't disappear.

Fixes #185.

[rob-c]

Hi Mark, I'm keen to get this in asap :)

Do you know from reading this code do we ever have a point where the 'data' file doesn't exist temporarily? I just wonder if a user hitting Ctrl+C at the wrong time would be exposed to this again a bit more?
(I know we can only do so much to prevent this)

[milliams]

Firstly the lock should be acquired and released using a context manager otherwise we will end up with dangling locks.

Secondly, do we really want a global lock or just one per object/file? If there any problem with writing job1 and job2 at the same time?

[drmarkwslater]

Stupid - now changed to using context manager!

For the moment, I would prefer (and it looks like @rob-c agrees) to use a global lock so we can be absolutely sure this has gone away. This may hit performance a little but hopefully not a lot. After we've got more testing for these things, then I would suggest revisiting it. E.g. I suspect this lock won't b needed anyway given the locking that's being added for the monitoring but better to be safe than sorry at the moment :)

Btw, I've added a safe_save unit test that recreates the bug to this PR as well.

[milliams]

For consistency with the others, let's call these GitHub185.

[milliams]

Also, since this doesn't use any of the Ganga runtime machinery I'd rather this was done as a standard UnitTest, not in the GPI directory. In a while I'm going to have to rearrange these anyway but if this is already in the right pace then it will be easier.

In fact, I would just call this test_safe_save and keep it as a generic unit test for the function rather than tying it to a specific issue.

[drmarkwslater]

Where is the place to put this to make sure it's run then? It certainly doesn't need the GPI but from what I could glean from Jenkins, we're only running tests in new_tests/GPI...

[drmarkwslater]

By tests I mean the PR tests!

[milliams]

You're right, it was just running over new_tests/GPI but I've just changed it so that it runs over all of new_tests so this will include all unit tests too. I'm not sure why I hadn't set it up like that before.

[drmarkwslater]

Forgot to say: the code to move the data file to data~ and moving the data.new file to data is straight after each other. If someone Ctrl+C's out they would have to be very unlucky but could then also recover from data.new anyway.

[rob-c]

OK, Erm We should probably change the load mechanism to check for a data.new existing as I think at the moment if it fails to load data it then goes directly to data~ which will miss out data.new. Although this feels like splitting hairs as we'd be quite unlucky to get this.

[drmarkwslater]

I agree - it's a minor point but should be made of a note of :)

[drmarkwslater]

I think that addresses everything - if so, let me know and I'll merge, or @milliams as release manager can do the honours :)

[milliams]

There's a brief problem with the Jenkins tests which I'm working to resolve before we can test and merge this. I'll let you know.

[milliams]

I'm happy to merge but we should bear in mind the future potential for a per-object lock (or perhaps it becomes unnecessary with the GangaObject-level locking?)

👍

[rob-c]

@milliams There is a per-object lock in the Registry which only works per-object per registry and doesn't protect against the bug this is fixing.
The locks in the GangaObject should prevent objects being modified during flushing whilst allow for appropriate getter methods to work.