BZ 69781 - improve FileStore thread safety

[aogburn]

No description provided.

[ChristopherSchultz]

There is slight disagreement between load, save, and remove as to when the locks are acquired. In some cases, the lock is acquired first and then file operations are performed, and in others some file operations are performed first, then the lock is acquired.

Is there a specific reason for that?

If not, please make all operations uniform in their operation.

[aogburn]

Thanks for the feedback @ChristopherSchultz. That primarily happened as I was trying to ensure that load obtained and kept its lock from the time it confirmed the file exists through to the completion of its read since any delete/modification between could lead to an unexpected result. And I didn't add any extra exists() check originally so just grabbed the lock on the intro to load instead. That should look better now though.

[ChristopherSchultz]

I'm not sure I see a change. In remove(), File.exists is still called before obtaining the lock. I think you should obtain the lock first. If you disagree, that's fine, but please provide a justification.

Also, please don't force-push and overwrite the commits. It's fine to have a PR with 10 commits. It makes is much easier to review the follow-up changes you've made.

[aogburn]

We'll actually modify (remove) the session file here so we should acquire the lock to protect from modifying the file during any potential load that may be reading it.

[aogburn]

If we've confirmed the file exists for us to read, then we don't want it modified or deleted from this point through to the read completion. So we grab the lock before file.exists and hold to read completion. If we checked file.exists before the lock in save, then it's not truly protected from a delete/modification from another thread right after before the save then acquires the lock and completes its read.

[aogburn]

The save is not consuming or removing existing data so it does not care or need to check if the file exists or not already. We just need to acquire the lock before writing any actual data to ensure we don't modify the data during a load operations data read.

[markt-asf]

I've added a test case for this to main (12.0.x). I haven't looked at the proposed solution yet.

[rmaucher]

Since there's an issue, the main question is if the extra code belongs to the store. The main problem is that there are two entry points to the store: the PersistentManagerBase but also the PersistentValve (not good).

[markt-asf]

I took a look at the patch and it looks OK. I think there is some were similar locking code in web resources but we can look at refactoring once this patch is committed.

I also looked at whether this needed a store level fix and I don't think it does. The requirement is that the Store implementation needs to support concurrent calls to load(), save() and remove(). I think it would be sufficient to document that requirement in the Store interface - another potential task after this patch is committed.

Any objections to accepting this PR at this point?

[rmaucher]

I took a look at the patch and it looks OK. I think there is some were similar locking code in web resources but we can look at refactoring once this patch is committed.

I also looked at whether this needed a store level fix and I don't think it does. The requirement is that the Store implementation needs to support concurrent calls to load(), save() and remove(). I think it would be sufficient to document that requirement in the Store interface - another potential task after this patch is committed.

Any objections to accepting this PR at this point?

+1 then, I haven't had time to review it yet.

[markt-asf]

The test case has found an issue with this implementation. The issue is that usage increments/
decrements and the lock/unlock actions are not atomic. I'm going to look at the solution already being used in WebResources and see if some refactoring/reuse is possible.

[markt-asf]

Closing this PR as it doesn't fully address the concurrency issues. I have a fix based on this PR that does address them that I will apply and then back-port shortly.