accounts/keystore: fix cache update and TestUpdatedKeyfileContents fa…

[gbotrel]

fix cache update and TestUpdatedKeyfileContents failing on macOS.
TestUpdatedKeyfileContents was randomly failing on macOS. Root cause: after writing to a file and closing the file handle, we get the fs notify event, we scan the directory and the modified time of the file is not yet updated (and the scanAccount logic relies on that to update the cache), even after the 500ms delay in watch.go.
I believe using the EventInfo from notify library in watch.go:93 to avoid scanning directory and relying on modified time of files would be safer (the event contains the Modified / Created / Deleted file path, yet we scan the whole directory and compare modified times)

[fjl]

Maybe a better fix would be to use modTime.After(prevMtime) || modTime.Equal(prevMtime) on all platforms.
Does that sound good to you?

[gbotrel]

Yes if we want to avoid platform specific code, I believe the perf impact is negligible

[holiman]

This means rescanning the entire directory again, which is one of the basic problems I wanted to avoid. It is non-neglible when there are hundreds of thousands of accounts. So if there's any other way of doing it (e.g differebt lib), I'd prefer that. Alternatively, keep this fix as a mac only solution. My 5 c.

[gbotrel]

Does it? (= rescan the entire directory). My understanding is that this scan is now triggered only after a FS event is thrown. Meaning something did effectively change in the directory --> meaning iteration i+2 will likely update the new reference modified time:
if modTime.After(newMtime) { newMtime = modTime }

and all the files not changing their modTime won't be picked again.

This was a quick fix, I agree there are more elegant solutions (see first comment --> what about using EventInfo data that contains the path of the file that changed ? but that does change much more than just a line of code)

[fjl]

Please remove travis-related changes from this PR.

[gbotrel]

Did remove the guilty commits with a history rewrite, please ignore GitCop messages :-)

[holiman]

Does it? (= rescan the entire directory). My understanding is that this scan is now triggered only after a FS event is thrown. Meaning something did effectively change in the directory --> meaning iteration i+2 will likely update the new reference modified time:
if modTime.After(newMtime) { newMtime = modTime }

and all the files not changing their modTime won't be picked again.

So, the newMtime gets set to whatever file was most recently updated, of all files. So you're right, it will not rescan all of them, just the one(s) that have the latest modiifcation time.

However, that makes me question if this fix will work. Consider the following:

1. There are a bunch of files, A (mtime:1), B (mtime: 2) and C (mtime: 3).
2. A scan is made. fc.mtime becomes set to 3 (meaning we needn't bother reading files with mtime before 3).
3. A is modified, and a rescan is made before the mtime of A is updated.
4. The file C will be read again, but not A.

Or is there another fs-event triggered once the mtime is changed?

[holiman]

Could the problem be that the mtime has only resolution on seconds? Another PR was merged which changed times to 1s in the tests. So this might be fixed already

[holiman]

This one: #15498.

[gbotrel]

The other PR don't seem to fix the underlying problem --> the test will pass, but the scenario we mentioned here can still happen and result in a cache not being updated. I agree with your previous comment, my fix is not enough for this to be bulletproof, what about using the EventInfo from notify library? If I have some time will run some tests the coming days.

[holiman]

I'll make a PR using the EventInfo thing, and hopefully the buildsystem can tell us how cross-platform portable it is.

[holiman]

@gbotrel I can't reproduce the issues myself, but if you're on a Mac, you're more than welcome to test my PR (linked above) and see if it fixes the issue for you.

[gbotrel]

@holiman --> did test your PR #15580 on mac, runs fine 👍 . But it has the other PR (#15498 ) changes in it, making the test quite longer than it was.
I did remove the multiple sleeps

// needed so that modTime of `file` is different to its current value after ioutil.WriteFile
// time.Sleep(1000 * time.Millisecond)

And the tests pass. I believe adding the sleeps was just hiding the test failure, and your PR solves the actual underlying problem.

Closing this PR .

[holiman]

Great, thanks for reporting! I'll change those values back, and clean up the PR, and mark it as no-longer-wip.