Merge pull request quarkusio#31102 from scrocquesel/codegen_race_cond…

…ition

Fix gRPC codegen race condition

core/deployment/src/main/java/io/quarkus/deployment/dev/CodeGenLock.java

@@ -0,0 +1,26 @@
+package io.quarkus.deployment.dev;
+
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+
+/**
+ * Lock that is used to prevent scanning and compiling while code generator is updating sources
+ * There is a race when testing this, where you can see the intermediate empty state of the
+ * file, or where the file time changes twice. Codegen hold this lock during modification
+ * to avoid the race.
+ */
+public class CodeGenLock {
+
+    /**
+     * Allow for multiple code generators to run at the same time but give exclusive run to RuntimeUpdatesProcessor
+     */
+    private static final ReentrantReadWriteLock lock = new ReentrantReadWriteLock(true);
+
+    public static Lock lockForCodeGen() {
+        return lock.readLock();
+    }
+
+    public static Lock lockForCompilation() {
+        return lock.writeLock();
+    }
+}

core/deployment/src/main/java/io/quarkus/deployment/dev/CodeGenWatcher.java

@@ -4,6 +4,7 @@
 import java.util.Collection;
 import java.util.List;
 import java.util.Properties;
+import java.util.concurrent.locks.Lock;
 
 import org.eclipse.microprofile.config.Config;
 import org.jboss.logging.Logger;
@@ -22,6 +23,7 @@ class CodeGenWatcher {
 
     private final QuarkusClassLoader deploymentClassLoader;
     private final FSWatchUtil fsWatchUtil;
+    private final Lock codeGenLock = CodeGenLock.lockForCodeGen();
 
     CodeGenWatcher(CuratedApplication curatedApplication, DevModeContext context) throws CodeGenException {
         final QuarkusClassLoader deploymentClassLoader = curatedApplication.createDeploymentClassLoader();
@@ -39,12 +41,15 @@ class CodeGenWatcher {
             for (CodeGenData codeGen : codeGens) {
                 watchers.add(new FSWatchUtil.Watcher(codeGen.sourceDir, codeGen.provider.inputExtension(),
                         modifiedPaths -> {
+                            codeGenLock.lock();
                             try {
                                 CodeGenerator.trigger(deploymentClassLoader,
                                         codeGen,
                                         curatedApplication.getApplicationModel(), config, false);
                             } catch (Exception any) {
                                 log.warn("Code generation failed", any);
+                            } finally {
+                                codeGenLock.unlock();
                             }
                         }));
             }

core/deployment/src/main/java/io/quarkus/deployment/dev/RuntimeUpdatesProcessor.java

@@ -38,6 +38,7 @@
 import java.util.concurrent.ConcurrentSkipListSet;
 import java.util.concurrent.CopyOnWriteArrayList;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.locks.Lock;
 import java.util.concurrent.locks.ReentrantLock;
 import java.util.function.BiConsumer;
 import java.util.function.BiFunction;
@@ -107,6 +108,7 @@ public class RuntimeUpdatesProcessor implements HotReplacementContext, Closeable
     private final BiConsumer<DevModeContext.ModuleInfo, String> copyResourceNotification;
     private final BiFunction<String, byte[], byte[]> classTransformers;
     private final ReentrantLock scanLock = new ReentrantLock();
+    private final Lock codeGenLock = CodeGenLock.lockForCompilation();
 
     /**
      * The index for the last successful start. Used to determine if the class has changed its structure
@@ -442,6 +444,8 @@ public boolean doScan(boolean userInitiated, boolean forceRestart) {
             return false;
         }
         scanLock.lock();
+        codeGenLock.lock();
+
         try {
             final long startNanoseconds = System.nanoTime();
             for (Runnable step : preScanSteps) {
@@ -564,6 +568,7 @@ public boolean doScan(boolean userInitiated, boolean forceRestart) {
 
         } finally {
             scanLock.unlock();
+            codeGenLock.unlock();
         }
     }
 

extensions/grpc/codegen/src/main/java/io/quarkus/grpc/deployment/GrpcPostProcessing.java

@@ -1,14 +1,7 @@
 package io.quarkus.grpc.deployment;
 
 import java.io.File;
-import java.io.IOException;
-import java.nio.file.Files;
 import java.nio.file.Path;
-import java.nio.file.StandardCopyOption;
-import java.util.HashMap;
-import java.util.Map;
-import java.util.Map.Entry;
-import java.util.function.Consumer;
 
 import org.jboss.logging.Logger;
 
@@ -72,7 +65,6 @@ private boolean isEnabled(CodeGenContext context, String name, boolean def) {
 
     public void postprocess() {
         SourceRoot sr = new SourceRoot(root);
-        Map<Path, Path> changedFiles = new HashMap<Path, Path>();
         try {
             sr.parse("", new SourceRoot.Callback() {
                 @Override
@@ -84,19 +76,9 @@ public com.github.javaparser.utils.SourceRoot.Callback.Result process(Path local
                         if (unit.getPrimaryType().isPresent()) {
                             TypeDeclaration<?> type = unit.getPrimaryType().get();
                             postprocess(unit, type);
-
-                            // save to a temporary file first, then move all temporary unit files at the same time
-                            try {
-                                unit.setStorage(Files.createTempFile(null, null),
-                                        sr.getParserConfiguration().getCharacterEncoding())
-                                        .getStorage().get().save(sr.getPrinter());
-                            } catch (IOException ex) {
-                                throw new RuntimeException(ex);
-                            }
-
-                            changedFiles.put(unit.getStorage().get().getPath(), absolutePath);
-                            return Result.DONT_SAVE;
+                            return Result.SAVE;
                         }
+
                     } else {
                         // Compilation issue - report and skip
                         log.errorf(
@@ -108,33 +90,9 @@ public com.github.javaparser.utils.SourceRoot.Callback.Result process(Path local
                     return Result.DONT_SAVE;
                 }
             });
-
-            changedFiles.entrySet().forEach(new Consumer<Entry<Path, Path>>() {
-                @Override
-                public void accept(Entry<Path, Path> entry) {
-                    try {
-                        Files.move(entry.getKey(), entry.getValue(), StandardCopyOption.REPLACE_EXISTING);
-                    } catch (IOException ex) {
-                        throw new RuntimeException(ex);
-                    }
-                }
-            });
-            changedFiles.clear();
-
         } catch (Exception e) {
             // read issue, report and exit
             log.error("Unable to parse the classes generated using protoc - skipping gRPC post processing", e);
-        } finally {
-            changedFiles.entrySet().forEach(new Consumer<Entry<Path, Path>>() {
-                @Override
-                public void accept(Entry<Path, Path> e) {
-                    try {
-                        Files.deleteIfExists(e.getKey());
-                    } catch (IOException discard) {
-                        // Ignore it.
-                    }
-                }
-            });
         }
     }