Change filelock behaviour #1906

kderme · 2020-04-06T07:51:41Z

#1903

mrBliss

That was quick.

Can you test this on Windows?

mrBliss · 2020-04-06T08:11:51Z

ouroboros-consensus/src/Ouroboros/Consensus/Node/DbLock.hs

+createDBFolder :: Monad m => HasFS m h -> m ()
+createDBFolder hasFS = do
+    createDirectoryIfMissing hasFS True root
+    void $ hOpen hasFS pFile (WriteMode AllowExisting)


Quoting what you quoted in #1903 (comment):

[..], on Windows 'System.IO.openFile' for a lock file will fail when the lock is held

So this is wrong as it tries to open the lock file even if it's locked.

Question: does tryLockFile work when the file doesn't exist yet? If so, then that would be easier. Ah, see https://github.com/takano-akio/filelock/blob/9681ff960d2695cd67fe749edab0e2af06c6f829/System/FileLock/Internal/LockFileEx.hsc#L40

No, it's the content of the file that is locked, not the file as a whole. That's why the failure in the logs ocured on reading the file, not opening the file.

Note that the file locking functions in base only work on open files.

mrBliss · 2020-04-06T08:12:18Z

ouroboros-consensus/src/Ouroboros/Consensus/Node/DbLock.hs

+    root  = mkFsPath []
+    pFile = fsPathFromList ["dblock"]
+
+-- | We try to lock the db multiple times, before we give up and throw an


As I said in #1903 (comment), I wouldn't retry this until we know we need it.

UPDATE: I was mistaken, retries are needed.

Let's wait until we're sure: #1903 (comment)

If we use a timeout on a blocking lock acquire, we don't need retries.

mrBliss · 2020-04-06T08:12:49Z

ouroboros-consensus/src/Ouroboros/Consensus/Node/DbLock.hs

+    void $ hOpen hasFS pFile (WriteMode AllowExisting)
+  where
+    root  = mkFsPath []
+    pFile = fsPathFromList ["dblock"]


This will go in light of my comment above it, but otherwise should have used dbLockFile here.

ouroboros-consensus/src/Ouroboros/Consensus/Node/DbLock.hs

ouroboros-consensus/src/Ouroboros/Consensus/Storage/ImmutableDB/Impl.hs

mrBliss · 2020-04-06T08:50:17Z

ouroboros-consensus/src/Ouroboros/Consensus/Node/DbLock.hs

+          threadDelay $ (n + 1) * 100000
+          lockAttempt $ n + 1


If we go for retries, let's use exponential backoff, not linear. Also we should document the behaviour: first wait x millisec, then ..., upto ..., etc. And we should indeed trace something, but this requires using a Tracer, and it's a bit annoying to have to add an extra tracer (cardano-node will have to be aware of it) just to trace one message.

It looks like the base package implementation of file locking on both posix, Linux and Windows will support the blocking lock acquite being interrupted by a timeout (async exception).

This would allow for a simple scheme:

res <- timeout 2000000 $ hLock lockfile ExclusiveLock

http://hackage.haskell.org/package/base-4.12.0.0/docs/GHC-IO-Handle-Lock.html

And if we're only blocking for up to one or two seconds, we don't need any new tracing I think.
#1903 (comment)

dcoutts · 2020-04-06T21:30:15Z

IMHO, there's nothing wrong with using the same file for the magic id and the file lock. But if you do, certainly you must open the lock file, lock it and hold it open for the duration. Only when it's opened and locked can you try to read the content.

But it's also ok to separate these into different files.

coot · 2020-04-12T15:23:01Z

What I recently learned (while working on ghc-tags-plugin, which is also using flock locking mechanism available in base) that one also needs to fsync (or fdatasync) before closing the file handle; In a highly concurrent setting, I've seen another thread (system thread / process) not reading the written but not fsync-ed content - I am not sure if this related in any way to ChainDB though.

ouroboros-consensus/src/Ouroboros/Consensus/Node/DbLock.hs

mrBliss · 2020-04-17T13:01:38Z

ouroboros-consensus/src/Ouroboros/Consensus/Node/DbLock.hs

+import           Ouroboros.Consensus.Util.IOLike
+
+-- We use an empty file as a lock of the db. Some systems may delete the empty
+-- file when all its handles are closed. This is not an issue, since the file is


Some systems may delete the empty file when all its handles are closed.

Does Windows do this?

No, I haven't seen it on windows or linux, but it's possible to happen.

ouroboros-consensus/src/Ouroboros/Consensus/Node/DbLock.hs

mrBliss · 2020-04-17T13:09:21Z

ouroboros-consensus/src/Ouroboros/Consensus/Node/DbLock.hs

+    mlock <- timeout (Time.secondsToDiffTime 2) $ wait a
+    case mlock of
+      Nothing   -> throwM $ DbLocked lockFilePath
+      Just lock -> finally action (unlockFile lock)


There's some masking missing. The simplest approach is to write a acquireLock and a releaseLock function and use those in bracket, then bracket will take care of the masking for us.

@mrBliss what do you think should be masked?

What if an async exception is thrown after obtaining the lock and before finally? That's the thing with async exceptions, they can happen at any point.

I'm not sure if we should mask while waiting on a timeout. Let me check it..

mrBliss · 2020-04-17T13:16:19Z

ouroboros-consensus/test-consensus/Test/Consensus/Node.hs

+    -- tryL (withLock touchLock) >>=
+    --     (@?= (Left (DbLocked (dbPath </> T.unpack dbLockFile))))


This test is commented out

Yes, as I explain above the test This test succeeds, but may leave a forgotten lock to the file and this lock is only cleaned when all tests finish and the process dies. So probably we shouldn't include it?

Yes, as I explain above the test This test succeeds, but may leave a forgotten lock to the file and this lock is only cleaned when all tests finish and the process dies. So probably we shouldn't include it?

But now we are not even testing whether the locking actually works.

this lock is only cleaned when all tests finish and the process dies

That's fine, or what's wrong with that?

ouroboros-consensus/src/Ouroboros/Consensus/Storage/ImmutableDB/Impl.hs

ouroboros-consensus/src/Ouroboros/Consensus/Node/DbLock.hs

kderme · 2020-04-22T08:41:25Z

I tested flock from base, but I couldn't make it work properly. It just wouldn't lock the files effectively and I din't manage to get down to why exactly this happened. So I used the filelock package. Also I preferred to use a different file from the DBMarker, since filelock suggests against accessing the locked file for other purposes.

mrBliss · 2020-04-22T14:36:12Z

ouroboros-consensus/src/Ouroboros/Consensus/Node/DbLock.hs

+withLockDB hasFS dbPath action = do
+    createDirectoryIfMissing hasFS True root
+    -- We want to avoid blocking the main thread at an uninterruptible ffi, to
+    -- avoid unresponsiveness to timeouts and ^C. So we use async and let a new


Suggested change

-- avoid unresponsiveness to timeouts and ^C. So we use async and let a new

-- avoid unresponsiveness to timeouts and ^C. So we use 'async' and let a new

mrBliss · 2020-04-22T14:36:24Z

ouroboros-consensus/src/Ouroboros/Consensus/Node/DbLock.hs

+    -- avoid unresponsiveness to timeouts and ^C. So we use async and let a new
+    -- thread do the actual ffi call.
+    --
+    -- We shouldn't be tempted to use `withAsync`, which is usually mentioned


Suggested change

-- We shouldn't be tempted to use `withAsync`, which is usually mentioned

-- We shouldn't be tempted to use 'withAsync', which is usually mentioned

mrBliss · 2020-04-22T14:36:41Z

ouroboros-consensus/src/Ouroboros/Consensus/Node/DbLock.hs

+    -- We shouldn't be tempted to use `withAsync`, which is usually mentioned
+    -- as a better alternative, or try to synchronously cancel the forked
+    -- thread during cleanup, since this would block the main thread and negate
+    -- the whole point of using async. We try our best to clean resources, but


Suggested change

-- the whole point of using async. We try our best to clean resources, but

-- the whole point of using 'async'. We try our best to clean up resources, but

mrBliss · 2020-04-22T14:39:25Z

ouroboros-consensus/src/Ouroboros/Consensus/Node/DbLock.hs

+      Nothing   -> throwM $ DbLocked lockFilePath
+      Just lock -> unlockFile lock


This pattern match should be done in the acquisition of the lock so that we throw that exception in case of a timeout, instead of passing Nothing to the action, which ignores it. This also means that cleanup will be just unlockFile.

@mrBliss I'm not sure I understand. This pattern matches if we got a timeout or async returned succesfully. So it can't be performed by the async thread.

When waiting on the async that calls lockFile times out, Nothing is returned. This is Nothing is then passed to (\_ -> action) which ignores it. So even when we don't have the lock do we execute the action. After the action is done or when action throws an exception, we go to cleanup. cleanup then throws a DbLocked exception in case it got Nothing, which is too late, as we already executed the action.

What we should do is (untype-checked):

createDirectoryIfMissing hasFS True root bracket acquireLock unLockFile (const action) where -- We want to avoid blocking ... acquireLock :: IO FileLock acquireLock = do lockFileAsync <- async (lockFile lockFilePath Exclusive) mbLock <- timeout (Time.secondsToDiffTime 2) $ wait lockFileAsync case mbLock of -- We timed out while waiting on the lock, the db is still locked Nothing -> throwM $ DbLocked lockFilePath Just lock -> return lock

oh right, fixed

mrBliss · 2020-04-22T14:41:26Z

ouroboros-consensus/src/Ouroboros/Consensus/Node/DbLock.hs

+
+-- | We use an empty file as a lock of the db so that the database cannot be
+-- opened by more than one process. We wait up to 2 seconds to take the lock,
+-- before timeout. Some systems may delete the empty file when all its handles


Suggested change

-- before timeout. Some systems may delete the empty file when all its handles

-- before timing out and throwing a 'DbLocked' exception. Some systems may delete the empty file when all its handles

mrBliss · 2020-04-22T14:54:30Z

Also I preferred to use a different file from the DBMarker, since filelock suggests against accessing the locked file for other purposes.

Yeah, I agree, that's a good reason.

mrBliss · 2020-04-23T08:08:06Z

(I have temporarily added the commits from #1851 to this branch to do a final check on Windows. I won't merge these commits, because building/testing everything on Windows is too slow because of the lack of caching.)

mrBliss · 2020-04-23T13:44:18Z

bors merge

1906: Change filelock behaviour r=mrBliss a=kderme #1903 Co-authored-by: kderme <k.dermenz@gmail.com> Co-authored-by: Thomas Winant <thomas@well-typed.com>

iohk-bors · 2020-04-23T14:10:17Z

Build failed

ci/hydra:Cardano:ouroboros-network:required

mrBliss · 2020-04-23T14:56:35Z

bors merge

1906: Change filelock behaviour r=mrBliss a=kderme #1903 Co-authored-by: kderme <k.dermenz@gmail.com> Co-authored-by: Thomas Winant <thomas@well-typed.com>

iohk-bors · 2020-04-23T15:57:51Z

Build failed

buildkite/ouroboros-network

mrBliss · 2020-04-24T06:29:33Z

bors merge

1906: Change filelock behaviour r=mrBliss a=kderme #1903 Co-authored-by: kderme <k.dermenz@gmail.com> Co-authored-by: Thomas Winant <thomas@well-typed.com>

iohk-bors · 2020-04-24T07:30:46Z

Build failed

buildkite/ouroboros-network

ouroboros-consensus/src/Ouroboros/Consensus/Util/FileLocking.hs

ouroboros-consensus/ouroboros-consensus-test-infra/src/Test/Util/FileLocking.hs

ouroboros-consensus/test-consensus/Test/Consensus/Node.hs

edsko

Nice, @kderme good work on the FFI analysis!

I also really like how the new property looks now.

mrBliss · 2020-04-24T11:14:02Z

bors merge

iohk-bors · 2020-04-24T11:23:44Z

Build succeeded

kderme mentioned this pull request Apr 6, 2020

Rapid cardano-node restart causes node to crash due to file locking #1903

Closed

mrBliss added the consensus issues related to ouroboros-consensus label Apr 6, 2020

mrBliss suggested changes Apr 6, 2020

View reviewed changes

mrBliss reviewed Apr 6, 2020

View reviewed changes

kderme force-pushed the kderme/lockfile branch 3 times, most recently from 2db434d to 76c93c1 Compare April 16, 2020 07:46

mrBliss reviewed Apr 17, 2020

View reviewed changes

kderme force-pushed the kderme/lockfile branch 2 times, most recently from 169d469 to 5732e3d Compare April 22, 2020 14:30

mrBliss reviewed Apr 22, 2020

View reviewed changes

kderme force-pushed the kderme/lockfile branch from 5732e3d to 9626a35 Compare April 22, 2020 15:42

mrBliss force-pushed the kderme/lockfile branch from 9626a35 to 0a7bee1 Compare April 23, 2020 08:04

mrBliss force-pushed the kderme/lockfile branch from dfdcda6 to 0a7bee1 Compare April 23, 2020 13:18

mrBliss approved these changes Apr 23, 2020

View reviewed changes

mrBliss linked an issue Apr 23, 2020 that may be closed by this pull request

Rapid cardano-node restart causes node to crash due to file locking #1903

Closed

mrBliss mentioned this pull request Apr 23, 2020

Test file locking with lazy release #1996

Closed

iohk-bors bot added a commit that referenced this pull request Apr 23, 2020

Merge #1906

712f0df

1906: Change filelock behaviour r=mrBliss a=kderme #1903 Co-authored-by: kderme <k.dermenz@gmail.com> Co-authored-by: Thomas Winant <thomas@well-typed.com>

iohk-bors bot added a commit that referenced this pull request Apr 23, 2020

Merge #1906

f5d1315

1906: Change filelock behaviour r=mrBliss a=kderme #1903 Co-authored-by: kderme <k.dermenz@gmail.com> Co-authored-by: Thomas Winant <thomas@well-typed.com>

mrBliss force-pushed the kderme/lockfile branch from 0a7bee1 to a87a1ac Compare April 24, 2020 06:29

iohk-bors bot added a commit that referenced this pull request Apr 24, 2020

Merge #1906

9a35f9c

1906: Change filelock behaviour r=mrBliss a=kderme #1903 Co-authored-by: kderme <k.dermenz@gmail.com> Co-authored-by: Thomas Winant <thomas@well-typed.com>

mrBliss force-pushed the kderme/lockfile branch from 6d06fbf to 7ed5339 Compare April 24, 2020 08:02

mrBliss reviewed Apr 24, 2020

View reviewed changes

edsko approved these changes Apr 24, 2020

View reviewed changes

mrBliss force-pushed the kderme/lockfile branch from 7ed5339 to 09699ac Compare April 24, 2020 10:45

kderme and others added 2 commits April 24, 2020 12:46

Change filelock behaviour

e902609

Strengthen the lock tests

a8cb44d

mrBliss force-pushed the kderme/lockfile branch 2 times, most recently from 071f557 to eab641c Compare April 24, 2020 11:11

Parameterise DbLock over FileLock and test timeouts

99849fd

mrBliss force-pushed the kderme/lockfile branch from eab641c to 99849fd Compare April 24, 2020 11:12

mrBliss linked an issue Apr 24, 2020 that may be closed by this pull request

Test file locking with lazy release #1996

Closed

iohk-bors bot merged commit 521286c into master Apr 24, 2020

iohk-bors bot deleted the kderme/lockfile branch April 24, 2020 11:23

		-- tryL (withLock touchLock) >>=
		-- (@?= (Left (DbLocked (dbPath </> T.unpack dbLockFile))))

	-- avoid unresponsiveness to timeouts and ^C. So we use async and let a new
	-- avoid unresponsiveness to timeouts and ^C. So we use 'async' and let a new

	-- We shouldn't be tempted to use `withAsync`, which is usually mentioned
	-- We shouldn't be tempted to use 'withAsync', which is usually mentioned

	-- the whole point of using async. We try our best to clean resources, but
	-- the whole point of using 'async'. We try our best to clean up resources, but

		Nothing -> throwM $ DbLocked lockFilePath
		Just lock -> unlockFile lock

	-- before timeout. Some systems may delete the empty file when all its handles
	-- before timing out and throwing a 'DbLocked' exception. Some systems may delete the empty file when all its handles

Change filelock behaviour #1906

Change filelock behaviour #1906

Uh oh!

Conversation

kderme commented Apr 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrBliss left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcoutts Apr 6, 2020 • 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!

mrBliss Apr 6, 2020 • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcoutts commented Apr 6, 2020

Uh oh!

coot commented Apr 12, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

kderme commented Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

kderme commented Apr 6, 2020 •

edited

Loading

dcoutts Apr 6, 2020 •

edited

Loading

mrBliss Apr 6, 2020 •

edited

Loading

kderme commented Apr 22, 2020 •

edited

Loading

mrBliss Apr 22, 2020 •

edited

Loading