[maximthomas]

Fixes #172.

CSVFileConnector.sync() now copies the CSV file under the read lock before the diff, diffs the token's snapshot against that copy, and keeps the copy as the next snapshot. A change that an external writer makes while a sync is running is now reported by the next sync instead of being lost.

Before this change, the index pass and the create/update pass read the live file. After the diff, if a delta had been accepted, the live file was copied to <csv>.<lastModified> as the next snapshot. External writers take no lock. A row deleted after the index pass was therefore in neither the diff nor the next snapshot, and no sync ever reported its DELETE.

• The mtime is read before the copy is made, and it names the next snapshot. If the copy were named after a later mtime, it could miss a change, and it would then be kept instead of the fresh copy of that mtime. Every sync would then report the change again.
• The copy is taken at the start of the read-locked section. The read lock keeps every writer in this JVM out of the copy, as the write lock would. Searches, gets and getLatestSyncToken keep running until a writer in this JVM queues behind the sync: the lock is a non-fair ReentrantReadWriteLock, which still makes a reader that starts after that wait behind the writer, for the rest of the copy and for the diff. Readers already wait this way during the diff.
• The new package-private copyCsvFile() writes the copy to a <csv>.<digits>.tmp file from createRewriteFile(), and deletes that file if the copy fails. The copy gets the mode of the CSV file, as snapshots do. Exception messages still name the CSV file, and the error log of a pass that reads the copy names the copy as well. A sync from a token still fails with ConnectorIOException "File … does not exist" when the CSV file is missing.
• If a delta was accepted, the copy is moved to <csv>.<mtime> with Files.move without options. If that snapshot already exists, the existing snapshot and the token stay, as before. In every other case the copy is deleted when the sync ends, whether it succeeded or failed.
• Unchanged:
  • the write-locked section, and the read lock during the diff;
  • the origin's token on every delta;
  • the copies.clear() after a handled delta ([#171] Skip CSV snapshot rows without a uid and delete the snapshots a failed sync copied #183);
  • the copy of the live file as the origin for a null token;
  • snapshot names;
  • exception texts, apart from an unreadable CSV file (see Trade-offs).

Trade-offs:

• Every sync copies the whole CSV file, even when nothing has changed, and needs free disk space for the copy. A sync with a null or stale token copies it twice: to its origin, as before, and for the diff. Writers in this JVM wait for the copy, as they wait for the diff. A search waits for the copy only if it starts after a writer has queued behind the sync, as it then waits for the diff. A sync of an unchanged file now fails with ConnectorException "Unable to copy CSV file for sync operation" if the connector cannot write to the directory or the disk is full. Before, such a sync returned no delta and failed only at the first change.
• A sync from a token now reads the CSV file first when it copies it, and only a missing file still fails with ConnectorIOException "File … does not exist". A CSV file that exists but cannot be read, or an I/O error while it is read, now fails with ConnectorException "Unable to copy CSV file for sync operation", as a sync without a token already did. Before, these were ConnectorIOException "File … does not exist" and "Error reading from file …".
• A copy can still be taken in the middle of a write by an external writer that does not write atomically.
• On POSIX, Files.move checks the target first and then renames, so it does not arbitrate between two syncs that reach the move with the same mtime: both can pass the check, and the second sync then replaces the first one's snapshot. Both copies were taken from the file at that mtime. If the second sync then fails in handleResult, it deletes the snapshot that the first sync handed out. This takes two syncs of one CSV file that do not share a lock: in two JVMs, or in one JVM through two absolute paths of the file, such as a symlink. Two syncs of one path in one JVM run one after another, because each starts with the write lock, which waits for the read lock of the other.
• A copy left behind by a killed process is not removed automatically, the same as for rewrites. Delete <csv>.<digits>.tmp files by hand while no connector is running against the CSV file.
• Out of scope:
  • an existing <csv>.<mtime> whose content differs from the live file at that mtime (CSVFile connector: sync() never deletes old snapshots, and fails or keeps the old token when the current file already has a snapshot #174);
  • a handler that stops the sync by returning false (CSVFile connector: sync() keeps offering deltas after the handler returns false and moves the token past the deltas it did not accept #175).

Test strength

arm	case	mutant killed	red at
CSVFileConnector.java:635 the create/update pass reads the copy	rowCreatedDuringSyncIsReportedByTheNextSyncOnly	pass reads the live file	78c1e16
CSVFileConnector.java:680 the copy becomes the next snapshot	rowDeletedDuringSyncIsReportedByTheNextSync	live file copied after the diff, as before	78c1e16
CSVFileConnector.java:676 same, once a delta is accepted	rowDeletedDuringSyncIsReportedByTheNextSync, rowCreatedDuringSyncIsReportedByTheNextSyncOnly, acceptedDeltaAdvancesTheToken	no move	78c1e16 (the first two)
CSVFileConnector.java:550 the index pass reads the copy	csvFileChangedAfterItsCopyIsReportedByTheNextSync (overrides copyCsvFile() to rewrite the live file once the copy is taken)	pass reads the live file	that mutant only (the seam is new)
CSVFileConnector.java:515 the mtime is read before the copy	same	mtime read after copyCsvFile()	that mutant only (the seam is new)
CSVFileConnector.java:517 the copy is taken under the read lock, so writers in this JVM wait for it, and a reader that starts before any writer queues does not	copyOfTheCsvFileBlocksWritersButNotReaders (overrides copyCsvFile() to call getLatestSyncToken, then delete, on another thread)	per mutant: the copy under the write lock, as at 4cffb1c (the reader times out after 10 s); the read lock released around the copy (the writer does not wait)	4cffb1c (the first mutant)
CSVFileConnector.java:680 an existing snapshot is not replaced	existingSnapshotOfTheCopiedMtimeKeepsTheToken	REPLACE_EXISTING	that mutant only (green at 78c1e16)
CSVFileConnector.java:685 FileAlreadyExistsException keeps the token	same	no catch	that mutant only (green at 78c1e16)
CSVFileConnector.java:707 the copy is deleted when the sync ends	existingSnapshotOfTheCopiedMtimeKeepsTheToken, successfulSyncKeepsTheSnapshotItCopied and 6 more	no delete	that mutant only
CSVFileConnector.java:1313 copyCsvFile() deletes its file when the copy fails	missingCsvFileFailsSync, unreadableCsvFileFailsSync	no delete	that mutant only (green at 78c1e16)
CSVFileConnector.java:688 a copy that cannot be moved to the next snapshot fails the sync	snapshotThatCannotBeMovedInFailsSync (POSIX, non-root)	no throw, so the old token is handed out	that mutant only (green at 78c1e16)
CSVFileConnector.java:1309 the copy, and so the next snapshot, takes the mode of the CSV file	snapshotTakesTheModeOfTheCsvFile (POSIX, skipped when the umask gives the CSV file rw-------)	copy into the createTempFile file in place, which keeps rw-------	that mutant only (green at 78c1e16)
CSVFileConnector.java:521 an unreadable CSV file fails the copy with "Unable to copy…"	unreadableCsvFileFailsSync (POSIX, non-root)	AccessDeniedException sent to the ConnectorIOException arm	78c1e16 (the old exception)
CSVFileConnector.java:566, :661 a pass that cannot read the copy names the CSV file in its exception and the copy in its log	copyDeletedBeforeTheIndexPassFailsSyncNamingTheCsvFile, copyDeletedBeforeTheCreateUpdatePassFailsSyncNamingTheCsvFile	per pass: the exception names the copy; the log names only the CSV file	that mutant only (the seam is new)
CSVFileConnector.java:682 next = null after the move	—	not pinned: without it, the delete after the read lock is released could remove a writer's .tmp file that createTempFile gave the freed name; a test cannot make it hand out that name	—

mvn -pl OpenICF-csvfile-connector test: 132 run, 0 failures, 0 skipped (122 at 78c1e16).

[vharseko]

CSVFileConnector.java:500: the copy of the CSV file does not need the write lock

next is a private .tmp file. Every writer in this JVM takes the write lock, so the read lock already keeps them out while the copy is taken. Holding the write lock for the copy instead blocks every search, get, getLatestSyncToken and the start of other syncs for the whole copy. Before this PR, that happened only on a sync with a null or stale token. Now it happens on every sync, including one from a valid token on an unchanged file. That path used to hold the write lock only for an exists() check.

Taking nextTimestamp and copyCsvFile() at the start of the read-locked section would avoid this, and needs no new error path: the finally at :698-710 already deletes copies and next when the sync fails. It would also take the second full copy that a sync with a null or stale token now makes (:489, then :500) out of the write-locked section.

[maximthomas]

Re review 5515753375: fixed in 2aa8447.

nextTimestamp and copyCsvFile() now run at the start of the read-locked section (CSVFileConnector.java:515-523). The write-locked section is the same as on master again: it picks the origin, copies the live file to it for a null or stale token, and calls scrubSyncFiles(). A failed copy needs no new cleanup: copyCsvFile() deletes its own file, and the finally of the read section deletes copies and next.

copyOfTheCsvFileBlocksWritersButNotReaders pins both sides of the lock. Its copyCsvFile() override calls getLatestSyncToken on another thread, which must return within 10 s. It then starts a delete there, which must still be waiting after 500 ms, and the next sync reports that delete. With the copy back under the write lock (the 4cffb1c code), the reader times out. With the read lock released around the copy, the delete does not wait. I re-ran the mutants of the rows the move touched: mtime read after the copy, no delete of next at the end, copyCsvFile() keeping its file, AccessDeniedException sent to the ConnectorIOException arm. The same tests still kill each of them. The module runs 132 tests, 0 failures, 0 skipped.

The move does not let another sync start earlier: it starts with the write lock, which waits for the read lock of the first sync until that sync ends. Following that through, the POSIX Files.move trade-off in the PR body over-claimed: two syncs of one path in one JVM never reach the move together. The body now says the race takes two syncs that do not share a lock, in two JVMs or through two absolute paths of the file in one JVM. Trade-offs now also list the second copy that a sync with a null or stale token makes, and the test-strength table has the new row and the line numbers at 2aa8447.

[vharseko]

Re 2aa8447: the copy now runs at the start of the read-locked section, and the write-locked section is back to what it is on master. A failed copy is cleaned up by copyCsvFile() itself, and the origin copy of a null or stale token by the finally of the read section. getLatestSyncToken, which can now run during the copy, does not pick up the .tmp file, because its pattern requires the name to end in 13 digits. copyOfTheCsvFileBlocksWritersButNotReaders kills both lock mutants. The corrected Files.move trade-off and the new table row match the code. The module passes locally: 132 tests, 0 failures, 0 skipped.

Non-blocking, wording only: the lock from fileNameToLockMap is a non-fair ReentrantReadWriteLock. Once a writer in this JVM queues behind the sync, a search that starts after it waits behind that writer: for the rest of the copy and for the diff. "Searches do not wait for the copy" therefore holds until a writer queues. The diff on master already behaves this way, so this is not a regression and needs no code change. The comment at :512 and the Trade-offs line could say so, if you want them exact.

[maximthomas]

Re review 5535943732: the wording is fixed in f1f2df9.

The comment at CSVFileConnector.java:512 now says that a reader which starts after a writer has queued also waits for the copy. The body says the same in three places: the bullet on the read-locked copy, the first Trade-offs line, and the :517 row of the table. I checked the lock behaviour on JDK 8, 11 and 26. While a read lock is held and a writer is queued, a new readLock().lock() blocks until both have released the lock.

copyOfTheCsvFileBlocksWritersButNotReaders is not affected: its reader finishes before the delete is submitted. The comment is still three lines long, so the line numbers in the body are unchanged. Only the comment changed, no code. The module passes locally: 132 tests, 0 failures, 0 skipped.

[vharseko]

Re f1f2df9: the comment at CSVFileConnector.java:512-514 now says that a reader which starts after a writer has queued also waits for the copy. The body says the same in the read-lock bullet, in the first Trade-offs line and in the :517 row of the table. All three match the code. Every reader in the connector takes the lock with lock() (:367, :723, :869), not with the barging tryLock(), so the non-fair queue check applies to it. On JDK 21 I see the same behaviour as the reply describes: while a read lock is held and a writer is queued, a new readLock().lock() waits until both have released the lock, and a reader that arrives before the writer gets in at once.

The commit changes only the comment. Its three lines stay within 120 columns, and the line numbers in the body still point at the statements they name. copyOfTheCsvFileBlocksWritersButNotReaders still tests the case it names: it awaits its getLatestSyncToken before it submits the delete. The module passes locally at f1f2df9: 132 tests, 0 failures, 0 skipped. CI is green.