perf(transformer): copy file content outside the FileTransformer lock#178
Merged
Conversation
FileTransformer.storeData held a single synchronized(this) monitor across both the target-name selection and the full content copy, so every crawler thread serialized on one lock for the entire I/O-bound file write. Reserve the target file atomically inside the lock (File.createNewFile with _N suffixes on collision, which also closes the previous check-then-act race) and perform CopyUtil.copy outside the lock, so concurrent threads write their files in parallel. Single-threaded naming and output are unchanged; the unreachable name-exhaustion case now throws a clearer CrawlerSystemException instead of failing later at write time.
…throws Add a test for the CrawlerSystemException that reserveFile throws when all maxDuplicatedPath candidate names are exhausted (previously uncovered), and correct storeData's @throws javadoc: I/O errors while copying the response body surface as IORuntimeException from CopyUtil, not wrapped here as CrawlerSystemException.
marevol
force-pushed
the
perf/crawler-concurrency-fixes
branch
from
81e7828 to
5b864f5
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
FileTransformer.storeDataheld onesynchronized (this)monitor across both the target-name selection and the full content copy. Because the transformer is a shared singleton, every crawler thread serialized on that single lock for the entire I/O-bound file write, so file-mirroring crawls had essentially no write concurrency. The name selection was also a check-then-act (exists()then pick) with a TOCTOU race.Changes
createFileis nowsynchronizedand reserves the leaf file atomically viaFile.createNewFile()(retryingname,name_0,name_1, … on collision). This both closes the TOCTOU race and keeps the critical section short (directory bookkeeping + one atomic create per candidate).storeDataperformsCopyUtil.copy(is, os)outside any lock, so concurrent threads write their files in parallel.Single-threaded behavior is unchanged: the same file path is chosen (
name, thenname_0, …) and the same content is written, with the same exception handling. The only edge-case change is the effectively-unreachable name-exhaustion path (maxDuplicatedPathcollisions), which now throws a clearCrawlerSystemExceptionfromcreateFileinstead of silently returning the parent directory and failing later when the stream is opened (still aCrawlerSystemException, just earlier and clearer).Testing
test_storeData_concurrent_sameUrl— 20 threads store the same URL concurrently; asserts 20 distinct files (name,name_0…name_18), each with its correct content and no extra file.test_storeData_concurrent_copyRunsOutsideLock— a barrier-gated input stream across 6 threads deterministically proves the copy is no longer serialized (this test times out/fails against the original locked-copy code and passes in ~0.1s with the fix).test_createFile/test_transformunchanged and passing; fullfess-crawlermodule suite passes (1980 tests);mvn formatter:format/license:format/javadoc:jarclean.