Lock cache metadata files for access #849

briandealwis · 2018-08-16T18:21:32Z

Acquire a file lock before retrieving and writing out the cache metadata file.

Note that this only serializes writing to the file but does not verify that the file has not been written to since it was initially read. The worst case is that some set of cached layers will be discarded and refetched on a subsequent build.

Fixes #848

coollog · 2018-08-16T18:47:49Z

jib-core/src/main/java/com/google/cloud/tools/jib/cache/Cache.java

-          JsonTemplateMapper.readJsonFromFile(cacheMetadataJsonFile, CacheMetadataTemplate.class);
-      return CacheMetadataTranslator.fromTemplate(cacheMetadataJson, cacheDirectory);
-
+      // channel is closed by inputStream.close()


Maybe we should include these as methods on JsonTemplateMapper/Blobs? Like JsonTemplateMapper#readJsonFromFile and Blobs#writeToFile?

briandealwis · 2018-08-17T19:57:26Z

Failed on Ubuntu:

com.google.cloud.tools.jib.gradle.JibPluginTest > testGetProjectDependencyAssembleTasks FAILED
    java.lang.ExceptionInInitializerError
        at java.util.concurrent.ConcurrentHashMap.fullAddCount(ConcurrentHashMap.java:2526)
        at java.util.concurrent.ConcurrentHashMap.addCount(ConcurrentHashMap.java:2266)
        at java.util.concurrent.ConcurrentHashMap.replaceNode(ConcurrentHashMap.java:1166)
        at java.util.concurrent.ConcurrentHashMap.remove(ConcurrentHashMap.java:1097)
        at org.mockito.internal.util.concurrent.WeakConcurrentMap.expungeStaleEntries(WeakConcurrentMap.java:127)
        at org.mockito.internal.util.concurrent.WeakConcurrentMap$WithInlinedExpunction.containsKey(WeakConcurrentMap.java:260)
        at org.mockito.internal.creation.bytebuddy.MockMethodAdvice.isMock(MockMethodAdvice.java:116)
        at java.net.URL.hashCode(URL.java:882)
        at java.security.CodeSource.hashCode(CodeSource.java:121)
        at java.util.HashMap.hash(HashMap.java:339)
        at java.util.HashMap.get(HashMap.java:557)
        at java.security.SecureClassLoader.getProtectionDomain(SecureClassLoader.java:204)
        at java.security.SecureClassLoader.defineClass(SecureClassLoader.java:142)
        at java.net.URLClassLoader.defineClass(URLClassLoader.java:467)
        at java.net.URLClassLoader.access$100(URLClassLoader.java:73)
        at java.net.URLClassLoader$1.run(URLClassLoader.java:368)
        at java.net.URLClassLoader$1.run(URLClassLoader.java:362)
        at java.security.AccessController.doPrivileged(Native Method)
        at java.net.URLClassLoader.findClass(URLClassLoader.java:361)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
        at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:349)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
        at org.gradle.testfixtures.internal.ProjectBuilderImpl.getGlobalServices(ProjectBuilderImpl.java:121)
        at org.gradle.testfixtures.internal.ProjectBuilderImpl.getUserHomeServices(ProjectBuilderImpl.java:114)
        at org.gradle.testfixtures.internal.ProjectBuilderImpl.createProject(ProjectBuilderImpl.java:91)
        at org.gradle.testfixtures.ProjectBuilder.build(ProjectBuilder.java:116)
        at com.google.cloud.tools.jib.gradle.JibPluginTest.testGetProjectDependencyAssembleTasks(JibPluginTest.java:91)

        Caused by:
        java.lang.NullPointerException
            at java.util.concurrent.ThreadLocalRandom.getProbe(ThreadLocalRandom.java:980)
            at java.util.concurrent.ConcurrentHashMap.fullAddCount(ConcurrentHashMap.java:2526)
            at java.util.concurrent.ConcurrentHashMap.addCount(ConcurrentHashMap.java:2266)
            at java.util.concurrent.ConcurrentHashMap.replaceNode(ConcurrentHashMap.java:1166)
            at java.util.concurrent.ConcurrentHashMap.remove(ConcurrentHashMap.java:1097)
            at org.mockito.internal.util.concurrent.WeakConcurrentMap.expungeStaleEntries(WeakConcurrentMap.java:127)
            at org.mockito.internal.util.concurrent.WeakConcurrentMap$WithInlinedExpunction.containsKey(WeakConcurrentMap.java:260)
            at org.mockito.internal.creation.bytebuddy.MockMethodAdvice.isMock(MockMethodAdvice.java:116)
            at org.mockito.internal.creation.bytebuddy.MockMethodAdvice.isMocked(MockMethodAdvice.java:121)
            at java.util.Hashtable.isEmpty(Hashtable.java:245)
            at sun.misc.VM.getSavedProperty(VM.java:258)
            at java.util.concurrent.ThreadLocalRandom.initialSeed(ThreadLocalRandom.java:139)
            at java.util.concurrent.ThreadLocalRandom.<clinit>(ThreadLocalRandom.java:136)
            ... 27 more

I hope this locking isn't tickling some underlying bug as the stack trace looks like JDK-8150014 (fixed in Java 9; dupes JDK-8150667, JDK-8150365).

coollog · 2018-08-17T20:14:34Z

jib-core/src/main/java/com/google/cloud/tools/jib/blob/Blob.java

+   * @return the {@link BlobDescriptor} of the written BLOB
+   * @throws IOException if writing the BLOB fails
+   */
+  default BlobDescriptor writeTo(Path jsonFile, boolean acquireLock) throws IOException {


Oh I was referring to placing the method in Blobs#writeToFile (similar to Blobs#writeToString and Blobs#writeToByteArray - Blobs is intended to be static methods that operate on Blob). This will help keep the Blob interface simple and avoid having to use default interface methods.

Ah, I didn't think of Blobs. Will look.

coollog · 2018-08-17T20:25:25Z

jib-core/src/main/java/com/google/cloud/tools/jib/json/JsonTemplateMapper.java

+  public static <T extends JsonTemplate> T readJsonFromFile(
+      Path jsonFile, Class<T> templateClass, boolean acquireLock) throws IOException {
+    if (!acquireLock) {
+      return objectMapper.readValue(Files.newInputStream(jsonFile), templateClass);


I think here we can either:

call out to the method without the extra acquireLock parameter, or

remove that other method without the acquireLock parameter, or

remove the acquireLock parameter and rename this method into something like readJsonFromFileWithLocking

briandealwis · 2018-08-17T20:40:00Z

Huh failed again. In the build logs, Maven reports the version as:

+ cd jib-maven-plugin
+ ./mvnw clean install -B -U -X
Apache Maven 3.5.2 (138edd61fd100ec658bfa2d307c43b76940a5d7d; 2017-10-18T00:58:13-07:00)
Maven home: /home/kbuilder/.m2/wrapper/dists/apache-maven-3.5.2-bin/28qa8v9e2mq69covern8vmdkj0/apache-maven-3.5.2
Java version: 1.8.0_171, vendor: Oracle Corporation
Java home: /usr/lib/jvm/java-8-oracle/jre

coollog · 2018-08-20T15:11:37Z

Did you experience failure again after the new commits?

briandealwis · 2018-08-20T15:16:51Z

Yeah, on MacOS this time. I don't see it locally. Maybe something fixed between 1.8.0_171 and _181.

coollog · 2018-08-20T19:01:07Z

jib-core/src/test/java/com/google/cloud/tools/jib/blob/BlobTest.java

+  @Test
+  public void testWriteToFileWithLock_newFile() throws IOException {
+    String expected = "crepecake";
+    File file = File.createTempFile("blob", "bin");


For consistency in this codebase, let's prefer java.nio for files over java.io - so Files.createTempFile and Files.delete

coollog · 2018-08-20T19:04:13Z

jib-core/src/test/java/com/google/cloud/tools/jib/blob/BlobTest.java

@@ -77,6 +108,7 @@ private void verifyBlobWriteTo(String expected, Blob blob) throws IOException {
    byte[] expectedBytes = expected.getBytes(StandardCharsets.UTF_8);
    Assert.assertEquals(expectedBytes.length, blobDescriptor.getSize());

+    @SuppressWarnings("resource") // no leak


We can actually change the mocked OutputStream to just Bytestreams.nullOutputStream() to avoid the suppression.

coollog · 2018-08-20T19:04:26Z

jib-core/src/test/java/com/google/cloud/tools/jib/json/JsonTemplateMapperTest.java

@@ -89,4 +90,33 @@ public void testWriteJson() throws DigestException, IOException, URISyntaxExcept

    Assert.assertEquals(expectedJson, jsonStream.toString());
  }
+
+  @Test
+  public void tedstReadJsonWithLock() throws IOException, URISyntaxException, DigestException {


s/tedst/test/

coollog · 2018-08-21T23:18:21Z

jib-core/src/test/java/com/google/cloud/tools/jib/blob/BlobTest.java

-    File file = File.createTempFile("blob", "bin");
-    Assert.assertTrue(file.delete()); // ensure it doesn't exist
+    Path blobFile = Files.createTempFile("blob", "bin");
+    Assert.assertTrue(Files.deleteIfExists(blobFile)); // ensure it doesn't exist


Wouldn't the file exist always?

I hope so. But Files.delete() is void, so at least this way we fail if something isn't right.

briandealwis added 2 commits August 16, 2018 14:10

Lock cache metadata files for access

c9cfbef

Jackson AUTO_CLOSE_SOURCE is on by default

8af821e

googlebot added the cla: yes label Aug 16, 2018

coollog reviewed Aug 16, 2018

View reviewed changes

Move locking into JsonTemplateMap and Blob

9d7367c

coollog reviewed Aug 17, 2018

View reviewed changes

briandealwis added 2 commits August 17, 2018 19:21

Shift implementations to Blobs

72f1538

Merge branch 'master' into i848

b8364aa

add tests

6f416c0

coollog reviewed Aug 20, 2018

View reviewed changes

review nits

4efa36d

coollog approved these changes Aug 21, 2018

View reviewed changes

Merge branch 'master' into i848

53b7436

briandealwis merged commit 02107ee into master Aug 22, 2018

briandealwis deleted the i848 branch August 22, 2018 02:19

briandealwis mentioned this pull request Sep 5, 2018

Occasional ThreadLocalRandom failures on Kokoro #940

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lock cache metadata files for access #849

Lock cache metadata files for access #849

briandealwis commented Aug 16, 2018

coollog Aug 16, 2018 •

edited

Loading

briandealwis commented Aug 17, 2018

coollog Aug 17, 2018

briandealwis Aug 17, 2018

coollog Aug 17, 2018

briandealwis commented Aug 17, 2018

coollog commented Aug 20, 2018

briandealwis commented Aug 20, 2018

coollog Aug 20, 2018

coollog Aug 20, 2018

coollog Aug 20, 2018

coollog Aug 21, 2018

briandealwis Aug 22, 2018

Lock cache metadata files for access #849

Lock cache metadata files for access #849

Conversation

briandealwis commented Aug 16, 2018

coollog Aug 16, 2018 • edited Loading

Choose a reason for hiding this comment

briandealwis commented Aug 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briandealwis commented Aug 17, 2018

coollog commented Aug 20, 2018

briandealwis commented Aug 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coollog Aug 16, 2018 •

edited

Loading