Repository navigation
Make ValidatesRunner faster: Improvements to file staging - #40320
Conversation
reuvenlax
changed the title
| return false; | ||
| } | ||
| FileHashCacheKey that = (FileHashCacheKey) o; | ||
| return length == that.length |
There was a problem hiding this comment.
Technically this isn't a true "equal" but just for file of same path, length, and same modified time, it will be treated equal.
We had a couple of security reports about file staging recently not sure if this would have security implications
Again, setting a retention time for the cache may mitigate potential conflicts
There was a problem hiding this comment.
unclear how this would be exploited. any exploit means that an attacker has full write access to the staging bucket, in which case they could anyway overwrite any binary with malicious code.
| .put(ENVIRONMENT_PROCESS, ImmutableSet.of(processCommandOption, processVariablesOption)) | ||
| .build(); | ||
|
|
||
| private static final ConcurrentHashMap<FileHashCacheKey, HashCode> FILE_HASH_CACHE = |
There was a problem hiding this comment.
In general, consider a bounded size cache in case of long running JVMs. One can use Guava's com.google.common.cache.Cache with maximumSize. Same applies to #40321
| return cachedZip; | ||
| } | ||
| try { | ||
| File zipFile = File.createTempFile(directory.getName(), ".zip"); |
There was a problem hiding this comment.
(Note: this fixed a pre-existing temp file leak which is good)
An attempt to better parallelize ValidatesRunner turned up a number of problems in our file staging.
1, Cache SHA-256 file hashes and directory zips
Fix calls ClassGraph.getClasspathFiles() directly instead of .scan(1).getClasspathFiles(). Also caches the resolves classpath, and uses a lock to ensure that only on thread resolve the classpath at a time (the other threads should then hit the cache)