From 4f3c53590c68542f4eb662a256e6808d4a71c384 Mon Sep 17 00:00:00 2001 From: Nathaniel Bauernfeind Date: Thu, 9 Nov 2023 22:13:18 -0700 Subject: [PATCH 1/6] QueryCompiler: Make JavaFileManager a Singleton --- .../engine/context/QueryCompiler.java | 53 ++++++++++++++++--- .../client/impl/TableServiceAsyncTest.java | 7 ++- 2 files changed, 52 insertions(+), 8 deletions(-) diff --git a/engine/context/src/main/java/io/deephaven/engine/context/QueryCompiler.java b/engine/context/src/main/java/io/deephaven/engine/context/QueryCompiler.java index aa4ca2f4137..50a94b8204e 100644 --- a/engine/context/src/main/java/io/deephaven/engine/context/QueryCompiler.java +++ b/engine/context/src/main/java/io/deephaven/engine/context/QueryCompiler.java @@ -683,19 +683,60 @@ private void maybeCreateClass(String className, String code, String packageName, } } + private static JavaCompiler JAVA_COMPILER = null; + + private static JavaCompiler getJavaCompiler() { + JavaCompiler localCompiler = JAVA_COMPILER; + if (localCompiler != null) { + return localCompiler; + } + synchronized (QueryCompiler.class) { + localCompiler = JAVA_COMPILER; + if (localCompiler != null) { + return localCompiler; + } + localCompiler = JAVA_COMPILER = ToolProvider.getSystemJavaCompiler(); + if (localCompiler == null) { + throw new RuntimeException("No Java compiler provided - are you using a JRE instead of a JDK?"); + } + } + return localCompiler; + } + + /** + * While the JavaFileManager should be closed to clean up resources, using a singleton avoids repeated processing of + * the classpath, which is very expensive. + */ + private static JavaFileManager JAVA_FILE_MANAGER = null; + + private static JavaFileManager getJavaFileManager() { + JavaFileManager localManager = JAVA_FILE_MANAGER; + if (localManager != null) { + return localManager; + } + synchronized (QueryCompiler.class) { + localManager = JAVA_FILE_MANAGER; + if (localManager != null) { + return localManager; + } + localManager = JAVA_FILE_MANAGER = getJavaCompiler().getStandardFileManager(null, null, null); + if (localManager == null) { + throw new RuntimeException("No Java compiler provided - are you using a JRE instead of a JDK?"); + } + } + return localManager; + } + private void maybeCreateClassHelper(String fqClassName, String finalCode, String[] splitPackageName, String rootPathAsString, String tempDirAsString) { final StringWriter compilerOutput = new StringWriter(); - final JavaCompiler compiler = ToolProvider.getSystemJavaCompiler(); - if (compiler == null) { - throw new RuntimeException("No Java compiler provided - are you using a JRE instead of a JDK?"); - } + final JavaCompiler compiler = getJavaCompiler(); final String classPathAsString = getClassPath() + File.pathSeparator + getJavaClassPath(); final List compilerOptions = Arrays.asList("-d", tempDirAsString, "-cp", classPathAsString); - final StandardJavaFileManager fileManager = compiler.getStandardFileManager(null, null, null); + final JavaFileManager fileManager = getJavaFileManager(); final boolean result = compiler.getTask(compilerOutput, fileManager, @@ -735,7 +776,7 @@ private void maybeCreateClassHelper(String fqClassName, String finalCode, String * @return a Pair of success, and the compiler output */ private Pair tryCompile(File basePath, Collection javaFiles) throws IOException { - final JavaCompiler compiler = ToolProvider.getSystemJavaCompiler(); + final JavaCompiler compiler = getJavaCompiler(); if (compiler == null) { throw new RuntimeException("No Java compiler provided - are you using a JRE instead of a JDK?"); } diff --git a/java-client/session-dagger/src/test/java/io/deephaven/client/impl/TableServiceAsyncTest.java b/java-client/session-dagger/src/test/java/io/deephaven/client/impl/TableServiceAsyncTest.java index 39089fc3b5c..5d0e0ea2fc1 100644 --- a/java-client/session-dagger/src/test/java/io/deephaven/client/impl/TableServiceAsyncTest.java +++ b/java-client/session-dagger/src/test/java/io/deephaven/client/impl/TableServiceAsyncTest.java @@ -19,7 +19,7 @@ public class TableServiceAsyncTest extends DeephavenSessionTestBase { private static final Duration GETTIME = Duration.ofSeconds(15); - private static final int CHAIN_OPS = 50; + private static final int CHAIN_OPS = 250; private static final int CHAIN_ROWS = 1000; @Test(timeout = 20000) @@ -93,8 +93,11 @@ private static List createLongChain(int numColumns, int numRows) { if (i == 0) { longChain.add(TableSpec.empty(numRows).view("I_0=ii")); } else { + // create a chain using the previous table even though we don't need the data final TableSpec prev = longChain.get(i - 1); - longChain.add(prev.updateView("I_" + i + " = 1 + I_" + (i - 1))); + // to avoid timeouts due to compilation variance reuse the same expression, including the destination + // column which shows up in the generated formula's FormulaEvaluationException + longChain.add(prev.updateView("I_1 = 1 + I_0")); } } return longChain; From ab71b89e704e99680bda8ad712e48a1038d9a0ae Mon Sep 17 00:00:00 2001 From: Nathaniel Bauernfeind Date: Fri, 10 Nov 2023 10:06:34 -0700 Subject: [PATCH 2/6] Devin's feedback --- .../engine/context/QueryCompiler.java | 43 ++++++++----------- .../client/impl/TableServiceAsyncTest.java | 37 ++++++++++------ .../deephaven/client/impl/BearerHandler.java | 7 +++ .../io/deephaven/client/impl/Session.java | 8 ++++ .../io/deephaven/client/impl/SessionImpl.java | 6 +++ .../runner/DeephavenApiServerTestBase.java | 20 +++++++-- 6 files changed, 79 insertions(+), 42 deletions(-) diff --git a/engine/context/src/main/java/io/deephaven/engine/context/QueryCompiler.java b/engine/context/src/main/java/io/deephaven/engine/context/QueryCompiler.java index 50a94b8204e..70740148cce 100644 --- a/engine/context/src/main/java/io/deephaven/engine/context/QueryCompiler.java +++ b/engine/context/src/main/java/io/deephaven/engine/context/QueryCompiler.java @@ -683,23 +683,20 @@ private void maybeCreateClass(String className, String code, String packageName, } } - private static JavaCompiler JAVA_COMPILER = null; + private static volatile JavaCompiler JAVA_COMPILER = null; private static JavaCompiler getJavaCompiler() { - JavaCompiler localCompiler = JAVA_COMPILER; - if (localCompiler != null) { - return localCompiler; - } - synchronized (QueryCompiler.class) { - localCompiler = JAVA_COMPILER; - if (localCompiler != null) { - return localCompiler; - } - localCompiler = JAVA_COMPILER = ToolProvider.getSystemJavaCompiler(); - if (localCompiler == null) { - throw new RuntimeException("No Java compiler provided - are you using a JRE instead of a JDK?"); + JavaCompiler localCompiler; + if ((localCompiler = JAVA_COMPILER) == null) { + synchronized (QueryCompiler.class) { + if ((localCompiler = JAVA_COMPILER) == null) { + localCompiler = JAVA_COMPILER = ToolProvider.getSystemJavaCompiler(); + } } } + if (localCompiler == null) { + throw new RuntimeException("No Java compiler provided - are you using a JRE instead of a JDK?"); + } return localCompiler; } @@ -707,21 +704,15 @@ private static JavaCompiler getJavaCompiler() { * While the JavaFileManager should be closed to clean up resources, using a singleton avoids repeated processing of * the classpath, which is very expensive. */ - private static JavaFileManager JAVA_FILE_MANAGER = null; + private static volatile JavaFileManager JAVA_FILE_MANAGER = null; private static JavaFileManager getJavaFileManager() { - JavaFileManager localManager = JAVA_FILE_MANAGER; - if (localManager != null) { - return localManager; - } - synchronized (QueryCompiler.class) { - localManager = JAVA_FILE_MANAGER; - if (localManager != null) { - return localManager; - } - localManager = JAVA_FILE_MANAGER = getJavaCompiler().getStandardFileManager(null, null, null); - if (localManager == null) { - throw new RuntimeException("No Java compiler provided - are you using a JRE instead of a JDK?"); + JavaFileManager localManager; + if ((localManager = JAVA_FILE_MANAGER) == null) { + synchronized (QueryCompiler.class) { + if ((localManager = JAVA_FILE_MANAGER) == null) { + localManager = JAVA_FILE_MANAGER = getJavaCompiler().getStandardFileManager(null, null, null); + } } } return localManager; diff --git a/java-client/session-dagger/src/test/java/io/deephaven/client/impl/TableServiceAsyncTest.java b/java-client/session-dagger/src/test/java/io/deephaven/client/impl/TableServiceAsyncTest.java index 5d0e0ea2fc1..ebf28d3013e 100644 --- a/java-client/session-dagger/src/test/java/io/deephaven/client/impl/TableServiceAsyncTest.java +++ b/java-client/session-dagger/src/test/java/io/deephaven/client/impl/TableServiceAsyncTest.java @@ -5,7 +5,12 @@ import io.deephaven.client.DeephavenSessionTestBase; import io.deephaven.client.impl.TableService.TableHandleFuture; +import io.deephaven.engine.context.ExecutionContext; +import io.deephaven.engine.table.Table; +import io.deephaven.engine.testutil.TstUtils; +import io.deephaven.engine.util.TableTools; import io.deephaven.qst.table.TableSpec; +import io.deephaven.util.SafeCloseable; import org.junit.Test; import java.time.Duration; @@ -27,7 +32,7 @@ public void longChainAsyncExportOnlyLast() throws ExecutionException, Interrupte final List longChain = createLongChain(); final TableSpec longChainLast = longChain.get(longChain.size() - 1); try (final TableHandle handle = get(session.executeAsync(longChainLast))) { - checkSucceeded(handle); + checkSucceeded(handle, CHAIN_OPS); } } @@ -36,9 +41,10 @@ public void longChainAsyncExportAll() throws ExecutionException, InterruptedExce final List longChain = createLongChain(); final List futures = session.executeAsync(longChain); try { + int chainLength = 0; for (final TableHandleFuture future : futures) { try (final TableHandle handle = get(future)) { - checkSucceeded(handle); + checkSucceeded(handle, ++chainLength); } } } catch (final Throwable t) { @@ -55,7 +61,7 @@ public void longChainAsyncExportAllCancelAllButLast() // Cancel or close all but the last one TableService.TableHandleFuture.cancelOrClose(futures.subList(0, futures.size() - 1), true); try (final TableHandle lastHandle = get(futures.get(futures.size() - 1))) { - checkSucceeded(lastHandle); + checkSucceeded(lastHandle, CHAIN_OPS); } } @@ -68,7 +74,7 @@ public void immediatelyCompletedFromCachedTableServices() try (final TableHandle ignored = get(tableService.executeAsync(longChainLast))) { for (int i = 0; i < 1000; ++i) { try (final TableHandle handle = get(tableService.executeAsync(longChainLast))) { - checkSucceeded(handle); + checkSucceeded(handle, CHAIN_OPS); } } } @@ -79,25 +85,30 @@ private static TableHandle get(TableHandleFuture future) return future.getOrCancel(GETTIME); } - private static void checkSucceeded(TableHandle x) { + private void checkSucceeded(TableHandle x, int chainLength) { assertThat(x.isSuccessful()).isTrue(); + try (final SafeCloseable ignored = getExecutionContext().open()) { + final Table result = getSession(session.getCurrentToken()).getExport(x.exportId().id()).get(); + ExecutionContext.getContext().getQueryScope().putParam("ChainLength", chainLength); + final Table expected = TableTools.emptyTable(CHAIN_ROWS).update("Current = ii - 1 + ChainLength"); + TstUtils.assertTableEquals(expected, result); + } } private static List createLongChain() { return createLongChain(CHAIN_OPS, CHAIN_ROWS); } - private static List createLongChain(int numColumns, int numRows) { - final List longChain = new ArrayList<>(numColumns); - for (int i = 0; i < numColumns; ++i) { + private static List createLongChain(int chainLength, int numRows) { + final List longChain = new ArrayList<>(chainLength); + for (int i = 0; i < chainLength; ++i) { if (i == 0) { - longChain.add(TableSpec.empty(numRows).view("I_0=ii")); + longChain.add(TableSpec.empty(numRows).view("Current = ii")); } else { - // create a chain using the previous table even though we don't need the data final TableSpec prev = longChain.get(i - 1); - // to avoid timeouts due to compilation variance reuse the same expression, including the destination - // column which shows up in the generated formula's FormulaEvaluationException - longChain.add(prev.updateView("I_1 = 1 + I_0")); + // Note: it's important that this formula is constant with respect to "i", otherwise we'll spend a lot + // of time compiling formulas + longChain.add(prev.updateView("Current = 1 + Current")); } } return longChain; diff --git a/java-client/session/src/main/java/io/deephaven/client/impl/BearerHandler.java b/java-client/session/src/main/java/io/deephaven/client/impl/BearerHandler.java index ce3d9a80fd5..538699222cd 100644 --- a/java-client/session/src/main/java/io/deephaven/client/impl/BearerHandler.java +++ b/java-client/session/src/main/java/io/deephaven/client/impl/BearerHandler.java @@ -1,5 +1,6 @@ package io.deephaven.client.impl; +import com.google.common.annotations.VisibleForTesting; import io.grpc.CallCredentials; import io.grpc.CallOptions; import io.grpc.Channel; @@ -14,6 +15,7 @@ import java.util.Objects; import java.util.Optional; +import java.util.UUID; import java.util.concurrent.Executor; import static io.deephaven.client.impl.Authentication.AUTHORIZATION_HEADER; @@ -56,6 +58,11 @@ public void setBearerToken(String bearerToken) { } } + @VisibleForTesting + UUID getCurrentToken() { + return UUID.fromString(bearerToken); + } + private void handleMetadata(Metadata metadata) { parseBearerToken(metadata).ifPresent(BearerHandler.this::setBearerToken); } diff --git a/java-client/session/src/main/java/io/deephaven/client/impl/Session.java b/java-client/session/src/main/java/io/deephaven/client/impl/Session.java index eaa050e2ca3..ae93d77429f 100644 --- a/java-client/session/src/main/java/io/deephaven/client/impl/Session.java +++ b/java-client/session/src/main/java/io/deephaven/client/impl/Session.java @@ -3,8 +3,10 @@ */ package io.deephaven.client.impl; +import com.google.common.annotations.VisibleForTesting; import io.deephaven.proto.DeephavenChannel; +import java.util.UUID; import java.util.concurrent.CompletableFuture; /** @@ -22,6 +24,12 @@ public interface Session @Override void close(); + /** + * Returns the current auth token. + */ + @VisibleForTesting + UUID getCurrentToken(); + /** * Closes the session. * diff --git a/java-client/session/src/main/java/io/deephaven/client/impl/SessionImpl.java b/java-client/session/src/main/java/io/deephaven/client/impl/SessionImpl.java index 7ef023b7e12..2d61c453a8e 100644 --- a/java-client/session/src/main/java/io/deephaven/client/impl/SessionImpl.java +++ b/java-client/session/src/main/java/io/deephaven/client/impl/SessionImpl.java @@ -42,6 +42,7 @@ import java.util.Map; import java.util.Objects; import java.util.Optional; +import java.util.UUID; import java.util.concurrent.CompletableFuture; import java.util.concurrent.ExecutionException; import java.util.concurrent.ScheduledExecutorService; @@ -132,6 +133,11 @@ private ExportStates newExportStates() { return new ExportStates(this, bearerChannel.session(), bearerChannel.table(), exportTicketCreator); } + @Override + public UUID getCurrentToken() { + return bearerHandler.getCurrentToken(); + } + @Override public TableService newStatefulTableService() { return new TableServiceImpl(newExportStates()); diff --git a/server/src/test/java/io/deephaven/server/runner/DeephavenApiServerTestBase.java b/server/src/test/java/io/deephaven/server/runner/DeephavenApiServerTestBase.java index 37679b44fa5..34aaab1b6d4 100644 --- a/server/src/test/java/io/deephaven/server/runner/DeephavenApiServerTestBase.java +++ b/server/src/test/java/io/deephaven/server/runner/DeephavenApiServerTestBase.java @@ -6,6 +6,7 @@ import dagger.BindsInstance; import dagger.Component; import io.deephaven.client.ClientDefaultsModule; +import io.deephaven.engine.context.ExecutionContext; import io.deephaven.engine.context.TestExecutionContext; import io.deephaven.engine.liveness.LivenessScope; import io.deephaven.engine.liveness.LivenessScopeStack; @@ -23,11 +24,13 @@ import io.deephaven.server.plugin.js.JsPluginNoopConsumerModule; import io.deephaven.server.runner.scheduler.SchedulerDelegatingImplModule; import io.deephaven.server.session.ObfuscatingErrorTransformerModule; +import io.deephaven.server.session.SessionState; import io.deephaven.server.util.Scheduler; import io.deephaven.util.SafeCloseable; import io.grpc.ManagedChannel; import io.grpc.ManagedChannelBuilder; import io.grpc.testing.GrpcCleanupRule; +import org.jetbrains.annotations.NotNull; import org.junit.After; import org.junit.Before; import org.junit.Rule; @@ -39,6 +42,7 @@ import java.io.PrintStream; import java.time.Duration; import java.util.Optional; +import java.util.UUID; import java.util.concurrent.TimeUnit; /** @@ -85,7 +89,8 @@ interface Builder { @Rule public final GrpcCleanupRule grpcCleanup = new GrpcCleanupRule(); - private SafeCloseable executionContext; + private ExecutionContext executionContext; + private SafeCloseable executionContextCloseable; private LogBuffer logBuffer; private SafeCloseable scopeCloseable; @@ -127,7 +132,8 @@ public void setUp() throws Exception { .injectFields(this); final PeriodicUpdateGraph updateGraph = server.getUpdateGraph().cast(); - executionContext = TestExecutionContext.createForUnitTests().withUpdateGraph(updateGraph).open(); + executionContext = TestExecutionContext.createForUnitTests().withUpdateGraph(updateGraph); + executionContextCloseable = executionContext.open(); if (updateGraph.isUnitTestModeAllowed()) { updateGraph.enableUnitTestMode(); updateGraph.resetForUnitTests(false); @@ -153,7 +159,7 @@ public void tearDown() throws Exception { if (updateGraph.isUnitTestModeAllowed()) { updateGraph.resetForUnitTests(true); } - executionContext.close(); + executionContextCloseable.close(); scheduler.shutdown(); } @@ -170,6 +176,14 @@ public ScriptSession getScriptSession() { return scriptSessionProvider.get(); } + public SessionState getSession(@NotNull final UUID token) { + return server.sessionService().getSessionForToken(token); + } + + public ExecutionContext getExecutionContext() { + return executionContext; + } + /** * The session token expiration * From 7606163d60393a25c29efaf8c958ee4f1aa70ee7 Mon Sep 17 00:00:00 2001 From: Nathaniel Bauernfeind Date: Fri, 10 Nov 2023 10:52:01 -0700 Subject: [PATCH 3/6] Devin's feedback rnd2 + synchronized compile --- .../engine/context/QueryCompiler.java | 22 +++++++++++-------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/engine/context/src/main/java/io/deephaven/engine/context/QueryCompiler.java b/engine/context/src/main/java/io/deephaven/engine/context/QueryCompiler.java index 70740148cce..1e29724cc1d 100644 --- a/engine/context/src/main/java/io/deephaven/engine/context/QueryCompiler.java +++ b/engine/context/src/main/java/io/deephaven/engine/context/QueryCompiler.java @@ -690,7 +690,7 @@ private static JavaCompiler getJavaCompiler() { if ((localCompiler = JAVA_COMPILER) == null) { synchronized (QueryCompiler.class) { if ((localCompiler = JAVA_COMPILER) == null) { - localCompiler = JAVA_COMPILER = ToolProvider.getSystemJavaCompiler(); + JAVA_COMPILER = localCompiler = ToolProvider.getSystemJavaCompiler(); } } } @@ -711,7 +711,7 @@ private static JavaFileManager getJavaFileManager() { if ((localManager = JAVA_FILE_MANAGER) == null) { synchronized (QueryCompiler.class) { if ((localManager = JAVA_FILE_MANAGER) == null) { - localManager = JAVA_FILE_MANAGER = getJavaCompiler().getStandardFileManager(null, null, null); + JAVA_FILE_MANAGER = localManager = getJavaCompiler().getStandardFileManager(null, null, null); } } } @@ -729,13 +729,17 @@ private void maybeCreateClassHelper(String fqClassName, String finalCode, String final JavaFileManager fileManager = getJavaFileManager(); - final boolean result = compiler.getTask(compilerOutput, - fileManager, - null, - compilerOptions, - null, - Collections.singletonList(new JavaSourceFromString(fqClassName, finalCode))) - .call(); + final boolean result; + // the java file manager is not thread safe + synchronized (QueryCompiler.class) { + result = compiler.getTask(compilerOutput, + fileManager, + null, + compilerOptions, + null, + Collections.singletonList(new JavaSourceFromString(fqClassName, finalCode))) + .call(); + } if (!result) { throw new RuntimeException("Error compiling class " + fqClassName + ":\n" + compilerOutput); } From 1fcb864467b6d771d27488d8de28d77d2b4a065e Mon Sep 17 00:00:00 2001 From: Nathaniel Bauernfeind Date: Fri, 10 Nov 2023 15:44:35 -0700 Subject: [PATCH 4/6] Devin rnd3 + Ryan rnd1 --- .../engine/context/QueryCompiler.java | 62 +++++++------------ .../client/DeephavenSessionTestBase.java | 12 +++- .../client/impl/TableServiceAsyncTest.java | 2 +- .../deephaven/client/impl/BearerHandler.java | 2 +- .../io/deephaven/client/impl/Session.java | 8 --- .../io/deephaven/client/impl/SessionImpl.java | 10 +-- .../server/runner/DeephavenApiServer.java | 2 +- .../runner/DeephavenApiServerTestBase.java | 7 --- 8 files changed, 36 insertions(+), 69 deletions(-) diff --git a/engine/context/src/main/java/io/deephaven/engine/context/QueryCompiler.java b/engine/context/src/main/java/io/deephaven/engine/context/QueryCompiler.java index 1e29724cc1d..c581e799e15 100644 --- a/engine/context/src/main/java/io/deephaven/engine/context/QueryCompiler.java +++ b/engine/context/src/main/java/io/deephaven/engine/context/QueryCompiler.java @@ -683,55 +683,23 @@ private void maybeCreateClass(String className, String code, String packageName, } } - private static volatile JavaCompiler JAVA_COMPILER = null; - - private static JavaCompiler getJavaCompiler() { - JavaCompiler localCompiler; - if ((localCompiler = JAVA_COMPILER) == null) { - synchronized (QueryCompiler.class) { - if ((localCompiler = JAVA_COMPILER) == null) { - JAVA_COMPILER = localCompiler = ToolProvider.getSystemJavaCompiler(); - } - } - } - if (localCompiler == null) { - throw new RuntimeException("No Java compiler provided - are you using a JRE instead of a JDK?"); - } - return localCompiler; - } - - /** - * While the JavaFileManager should be closed to clean up resources, using a singleton avoids repeated processing of - * the classpath, which is very expensive. - */ - private static volatile JavaFileManager JAVA_FILE_MANAGER = null; - - private static JavaFileManager getJavaFileManager() { - JavaFileManager localManager; - if ((localManager = JAVA_FILE_MANAGER) == null) { - synchronized (QueryCompiler.class) { - if ((localManager = JAVA_FILE_MANAGER) == null) { - JAVA_FILE_MANAGER = localManager = getJavaCompiler().getStandardFileManager(null, null, null); - } - } - } - return localManager; - } - private void maybeCreateClassHelper(String fqClassName, String finalCode, String[] splitPackageName, String rootPathAsString, String tempDirAsString) { final StringWriter compilerOutput = new StringWriter(); - final JavaCompiler compiler = getJavaCompiler(); + final JavaCompiler compiler = ToolProvider.getSystemJavaCompiler(); + if (compiler == null) { + throw new RuntimeException("No Java compiler provided - are you using a JRE instead of a JDK?"); + } final String classPathAsString = getClassPath() + File.pathSeparator + getJavaClassPath(); final List compilerOptions = Arrays.asList("-d", tempDirAsString, "-cp", classPathAsString); - final JavaFileManager fileManager = getJavaFileManager(); + final StandardJavaFileManager fileManager = compiler.getStandardFileManager(null, null, null); - final boolean result; - // the java file manager is not thread safe - synchronized (QueryCompiler.class) { + boolean result = false; + boolean exceptionThrown = false; + try { result = compiler.getTask(compilerOutput, fileManager, null, @@ -739,6 +707,18 @@ private void maybeCreateClassHelper(String fqClassName, String finalCode, String null, Collections.singletonList(new JavaSourceFromString(fqClassName, finalCode))) .call(); + } catch (final Exception err) { + exceptionThrown = true; + throw err; + } finally { + try { + fileManager.close(); + } catch (final IOException ioe) { + if (!exceptionThrown) { + // noinspection ThrowFromFinallyBlock + throw new UncheckedIOException("Could not close JavaFileManager", ioe); + } + } } if (!result) { throw new RuntimeException("Error compiling class " + fqClassName + ":\n" + compilerOutput); @@ -771,7 +751,7 @@ private void maybeCreateClassHelper(String fqClassName, String finalCode, String * @return a Pair of success, and the compiler output */ private Pair tryCompile(File basePath, Collection javaFiles) throws IOException { - final JavaCompiler compiler = getJavaCompiler(); + final JavaCompiler compiler = ToolProvider.getSystemJavaCompiler(); if (compiler == null) { throw new RuntimeException("No Java compiler provided - are you using a JRE instead of a JDK?"); } diff --git a/java-client/session-dagger/src/test/java/io/deephaven/client/DeephavenSessionTestBase.java b/java-client/session-dagger/src/test/java/io/deephaven/client/DeephavenSessionTestBase.java index b31e8d71eb7..d5ea32d9615 100644 --- a/java-client/session-dagger/src/test/java/io/deephaven/client/DeephavenSessionTestBase.java +++ b/java-client/session-dagger/src/test/java/io/deephaven/client/DeephavenSessionTestBase.java @@ -3,8 +3,11 @@ */ package io.deephaven.client; +import io.deephaven.base.verify.Require; import io.deephaven.client.impl.Session; +import io.deephaven.client.impl.SessionImpl; import io.deephaven.server.runner.DeephavenApiServerTestBase; +import io.deephaven.server.session.SessionState; import io.grpc.ManagedChannel; import org.junit.After; import org.junit.Before; @@ -17,6 +20,7 @@ public abstract class DeephavenSessionTestBase extends DeephavenApiServerTestBas private ScheduledExecutorService sessionScheduler; protected Session session; + protected SessionState serverSessionState; @Override @Before @@ -25,8 +29,12 @@ public void setUp() throws Exception { ManagedChannel channel = channelBuilder().build(); register(channel); sessionScheduler = Executors.newScheduledThreadPool(2); - session = DaggerDeephavenSessionRoot.create().factoryBuilder().managedChannel(channel) - .scheduler(sessionScheduler).build().newSession(); + final SessionImpl clientSessionImpl = + DaggerDeephavenSessionRoot.create().factoryBuilder().managedChannel(channel) + .scheduler(sessionScheduler).build().newSession(); + session = clientSessionImpl; + serverSessionState = Require.neqNull(server().sessionService().getSessionForToken( + clientSessionImpl._hackBearerHandler().getCurrentToken()), "SessionState"); } @Override diff --git a/java-client/session-dagger/src/test/java/io/deephaven/client/impl/TableServiceAsyncTest.java b/java-client/session-dagger/src/test/java/io/deephaven/client/impl/TableServiceAsyncTest.java index ebf28d3013e..1e8b7f188d7 100644 --- a/java-client/session-dagger/src/test/java/io/deephaven/client/impl/TableServiceAsyncTest.java +++ b/java-client/session-dagger/src/test/java/io/deephaven/client/impl/TableServiceAsyncTest.java @@ -88,7 +88,7 @@ private static TableHandle get(TableHandleFuture future) private void checkSucceeded(TableHandle x, int chainLength) { assertThat(x.isSuccessful()).isTrue(); try (final SafeCloseable ignored = getExecutionContext().open()) { - final Table result = getSession(session.getCurrentToken()).
getExport(x.exportId().id()).get(); + final Table result = serverSessionState.
getExport(x.exportId().id()).get(); ExecutionContext.getContext().getQueryScope().putParam("ChainLength", chainLength); final Table expected = TableTools.emptyTable(CHAIN_ROWS).update("Current = ii - 1 + ChainLength"); TstUtils.assertTableEquals(expected, result); diff --git a/java-client/session/src/main/java/io/deephaven/client/impl/BearerHandler.java b/java-client/session/src/main/java/io/deephaven/client/impl/BearerHandler.java index 538699222cd..8c1d3e3ff46 100644 --- a/java-client/session/src/main/java/io/deephaven/client/impl/BearerHandler.java +++ b/java-client/session/src/main/java/io/deephaven/client/impl/BearerHandler.java @@ -59,7 +59,7 @@ public void setBearerToken(String bearerToken) { } @VisibleForTesting - UUID getCurrentToken() { + public UUID getCurrentToken() { return UUID.fromString(bearerToken); } diff --git a/java-client/session/src/main/java/io/deephaven/client/impl/Session.java b/java-client/session/src/main/java/io/deephaven/client/impl/Session.java index ae93d77429f..eaa050e2ca3 100644 --- a/java-client/session/src/main/java/io/deephaven/client/impl/Session.java +++ b/java-client/session/src/main/java/io/deephaven/client/impl/Session.java @@ -3,10 +3,8 @@ */ package io.deephaven.client.impl; -import com.google.common.annotations.VisibleForTesting; import io.deephaven.proto.DeephavenChannel; -import java.util.UUID; import java.util.concurrent.CompletableFuture; /** @@ -24,12 +22,6 @@ public interface Session @Override void close(); - /** - * Returns the current auth token. - */ - @VisibleForTesting - UUID getCurrentToken(); - /** * Closes the session. * diff --git a/java-client/session/src/main/java/io/deephaven/client/impl/SessionImpl.java b/java-client/session/src/main/java/io/deephaven/client/impl/SessionImpl.java index 2d61c453a8e..683f78e472d 100644 --- a/java-client/session/src/main/java/io/deephaven/client/impl/SessionImpl.java +++ b/java-client/session/src/main/java/io/deephaven/client/impl/SessionImpl.java @@ -42,7 +42,6 @@ import java.util.Map; import java.util.Objects; import java.util.Optional; -import java.util.UUID; import java.util.concurrent.CompletableFuture; import java.util.concurrent.ExecutionException; import java.util.concurrent.ScheduledExecutorService; @@ -124,8 +123,8 @@ private SessionImpl(SessionImplConfig config, DeephavenChannel bearerChannel, Du pingFrequency.toNanos(), pingFrequency.toNanos(), TimeUnit.NANOSECONDS); } - // exposed for Flight - BearerHandler _hackBearerHandler() { + // exposed for Flight and testing + public BearerHandler _hackBearerHandler() { return bearerHandler; } @@ -133,11 +132,6 @@ private ExportStates newExportStates() { return new ExportStates(this, bearerChannel.session(), bearerChannel.table(), exportTicketCreator); } - @Override - public UUID getCurrentToken() { - return bearerHandler.getCurrentToken(); - } - @Override public TableService newStatefulTableService() { return new TableServiceImpl(newExportStates()); diff --git a/server/src/main/java/io/deephaven/server/runner/DeephavenApiServer.java b/server/src/main/java/io/deephaven/server/runner/DeephavenApiServer.java index 1288790f503..fa3de22157b 100644 --- a/server/src/main/java/io/deephaven/server/runner/DeephavenApiServer.java +++ b/server/src/main/java/io/deephaven/server/runner/DeephavenApiServer.java @@ -98,7 +98,7 @@ public GrpcServer server() { } @VisibleForTesting - SessionService sessionService() { + public SessionService sessionService() { return sessionService; } diff --git a/server/src/test/java/io/deephaven/server/runner/DeephavenApiServerTestBase.java b/server/src/test/java/io/deephaven/server/runner/DeephavenApiServerTestBase.java index 34aaab1b6d4..a064a1ef415 100644 --- a/server/src/test/java/io/deephaven/server/runner/DeephavenApiServerTestBase.java +++ b/server/src/test/java/io/deephaven/server/runner/DeephavenApiServerTestBase.java @@ -24,13 +24,11 @@ import io.deephaven.server.plugin.js.JsPluginNoopConsumerModule; import io.deephaven.server.runner.scheduler.SchedulerDelegatingImplModule; import io.deephaven.server.session.ObfuscatingErrorTransformerModule; -import io.deephaven.server.session.SessionState; import io.deephaven.server.util.Scheduler; import io.deephaven.util.SafeCloseable; import io.grpc.ManagedChannel; import io.grpc.ManagedChannelBuilder; import io.grpc.testing.GrpcCleanupRule; -import org.jetbrains.annotations.NotNull; import org.junit.After; import org.junit.Before; import org.junit.Rule; @@ -42,7 +40,6 @@ import java.io.PrintStream; import java.time.Duration; import java.util.Optional; -import java.util.UUID; import java.util.concurrent.TimeUnit; /** @@ -176,10 +173,6 @@ public ScriptSession getScriptSession() { return scriptSessionProvider.get(); } - public SessionState getSession(@NotNull final UUID token) { - return server.sessionService().getSessionForToken(token); - } - public ExecutionContext getExecutionContext() { return executionContext; } From 4abd696003de2196a08d3862355b694691fc0cb6 Mon Sep 17 00:00:00 2001 From: Nathaniel Bauernfeind Date: Fri, 10 Nov 2023 16:42:48 -0700 Subject: [PATCH 5/6] Ryan's feedback rnd2 --- .../engine/context/QueryCompiler.java | 24 ++++++++++--------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/engine/context/src/main/java/io/deephaven/engine/context/QueryCompiler.java b/engine/context/src/main/java/io/deephaven/engine/context/QueryCompiler.java index c581e799e15..b2785031412 100644 --- a/engine/context/src/main/java/io/deephaven/engine/context/QueryCompiler.java +++ b/engine/context/src/main/java/io/deephaven/engine/context/QueryCompiler.java @@ -113,7 +113,7 @@ private QueryCompiler( try { urls[0] = (classDestination.toURI().toURL()); } catch (MalformedURLException e) { - throw new RuntimeException("", e); + throw new UncheckedDeephavenException(e); } this.ucl = new WritableURLClassLoader(urls, parentClassLoaderToUse); @@ -183,7 +183,8 @@ public static void writeClass(final File destinationDirectory, final String clas ensureDirectories(parentDir, () -> "Unable to create missing destination directory " + parentDir.getAbsolutePath()); if (!destinationFile.createNewFile()) { - throw new RuntimeException("Unable to create destination file " + destinationFile.getAbsolutePath()); + throw new UncheckedDeephavenException( + "Unable to create destination file " + destinationFile.getAbsolutePath()); } final ByteArrayOutputStream byteOutStream = new ByteArrayOutputStream(data.length); byteOutStream.write(data, 0, data.length); @@ -274,7 +275,7 @@ private static void ensureDirectories(final File file, final Supplier ru // (and therefore mkdirs() would return false), but still get the directory we need (and therefore exists() // would return true) if (!file.mkdirs() && !file.isDirectory()) { - throw new RuntimeException(runtimeErrMsg.get()); + throw new UncheckedDeephavenException(runtimeErrMsg.get()); } } @@ -396,7 +397,7 @@ private void addClassSource(File classSourceDirectory) { try { ucl.addURL(classSourceDirectory.toURI().toURL()); } catch (MalformedURLException e) { - throw new RuntimeException("", e); + throw new UncheckedDeephavenException(e); } } @@ -425,7 +426,7 @@ private Class compileHelper(@NotNull final String className, try { digest = MessageDigest.getInstance("SHA-256"); } catch (NoSuchAlgorithmException e) { - throw new RuntimeException("Unable to create SHA-256 hashing digest", e); + throw new UncheckedDeephavenException("Unable to create SHA-256 hashing digest", e); } final String basicHashText = ByteUtils.byteArrToHex(digest.digest(classBody.getBytes(StandardCharsets.UTF_8))); @@ -645,7 +646,8 @@ private void maybeCreateClass(String className, String code, String packageName, final String[] splitPackageName = packageName.split("\\."); if (splitPackageName.length == 0) { - throw new RuntimeException(String.format("packageName %s expected to have at least one .", packageName)); + throw new UncheckedDeephavenException(String.format( + "packageName %s expected to have at least one .", packageName)); } final String[] truncatedSplitPackageName = Arrays.copyOf(splitPackageName, splitPackageName.length - 1); @@ -689,7 +691,7 @@ private void maybeCreateClassHelper(String fqClassName, String finalCode, String final JavaCompiler compiler = ToolProvider.getSystemJavaCompiler(); if (compiler == null) { - throw new RuntimeException("No Java compiler provided - are you using a JRE instead of a JDK?"); + throw new UncheckedDeephavenException("No Java compiler provided - are you using a JRE instead of a JDK?"); } final String classPathAsString = getClassPath() + File.pathSeparator + getJavaClassPath(); @@ -707,7 +709,7 @@ private void maybeCreateClassHelper(String fqClassName, String finalCode, String null, Collections.singletonList(new JavaSourceFromString(fqClassName, finalCode))) .call(); - } catch (final Exception err) { + } catch (final Throwable err) { exceptionThrown = true; throw err; } finally { @@ -721,7 +723,7 @@ private void maybeCreateClassHelper(String fqClassName, String finalCode, String } } if (!result) { - throw new RuntimeException("Error compiling class " + fqClassName + ":\n" + compilerOutput); + throw new UncheckedDeephavenException("Error compiling class " + fqClassName + ":\n" + compilerOutput); } // The above has compiled into e.g. // /tmp/workspace/cache/classes/temporaryCompilationDirectory12345/io/deephaven/test/cm12862183232603186v52_0/{various @@ -753,7 +755,7 @@ private void maybeCreateClassHelper(String fqClassName, String finalCode, String private Pair tryCompile(File basePath, Collection javaFiles) throws IOException { final JavaCompiler compiler = ToolProvider.getSystemJavaCompiler(); if (compiler == null) { - throw new RuntimeException("No Java compiler provided - are you using a JRE instead of a JDK?"); + throw new UncheckedDeephavenException("No Java compiler provided - are you using a JRE instead of a JDK?"); } final File outputDirectory = Files.createTempDirectory("temporaryCompilationDirectory").toFile(); @@ -844,7 +846,7 @@ private static String getJavaClassPath() { } } } catch (IOException e) { - throw new RuntimeException("Error extract manifest file from " + javaClasspath + ".\n", e); + throw new UncheckedIOException("Error extract manifest file from " + javaClasspath + ".\n", e); } } return javaClasspath; From b7b90240c7f323d6da9f41be3ec86322d2acd57d Mon Sep 17 00:00:00 2001 From: Nathaniel Bauernfeind Date: Fri, 10 Nov 2023 16:45:53 -0700 Subject: [PATCH 6/6] Quick variable type change to interface --- .../main/java/io/deephaven/engine/context/QueryCompiler.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/engine/context/src/main/java/io/deephaven/engine/context/QueryCompiler.java b/engine/context/src/main/java/io/deephaven/engine/context/QueryCompiler.java index b2785031412..6b54b9bb9fe 100644 --- a/engine/context/src/main/java/io/deephaven/engine/context/QueryCompiler.java +++ b/engine/context/src/main/java/io/deephaven/engine/context/QueryCompiler.java @@ -697,7 +697,7 @@ private void maybeCreateClassHelper(String fqClassName, String finalCode, String final String classPathAsString = getClassPath() + File.pathSeparator + getJavaClassPath(); final List compilerOptions = Arrays.asList("-d", tempDirAsString, "-cp", classPathAsString); - final StandardJavaFileManager fileManager = compiler.getStandardFileManager(null, null, null); + final JavaFileManager fileManager = compiler.getStandardFileManager(null, null, null); boolean result = false; boolean exceptionThrown = false;