deephaven / deephaven-core

Deephaven Community Core
Other
257 stars 80 forks source link

Support for referencing exports from a console session #5992

Open devinrsmith opened 2 months ago

devinrsmith commented 2 months ago

It could be useful to have the ability to reference exports from the script of ConsoleService.ExecuteCommand. This would allow a "hybrid execution" workflow where some table operations are exported via TableService and then referenced in ConsoleService.ExecuteCommand. This may be particularly useful for table operations that are not currently plumbed through RPCs:

myResultingTable = SomeCoolOperation.create(ExportUtil.getExport(42))

The alternative right now requires users to first bind their variable into the global scope via BindTableToVariableRequest (or PublishFromTicket):

// BindTableToVariableRequest has already been called, creating __tmp_export_abcxyz from export id 42
try {
  myResultingTable = SomeCoolOperation.create(__tmp_export_abcxyz)
} finally {
  __tmp_export_abcxyz = null
}

The following may be a viable way of exposing this functionality to ExecuteCommand

diff --git a/server/src/main/java/io/deephaven/server/console/ConsoleServiceGrpcImpl.java b/server/src/main/java/io/deephaven/server/console/ConsoleServiceGrpcImpl.java
index 8ab6115ccb..388ac3e20b 100644
--- a/server/src/main/java/io/deephaven/server/console/ConsoleServiceGrpcImpl.java
+++ b/server/src/main/java/io/deephaven/server/console/ConsoleServiceGrpcImpl.java
@@ -39,6 +39,7 @@ import io.deephaven.server.session.SessionState.ExportBuilder;
 import io.deephaven.server.session.TicketRouter;
 import io.deephaven.server.util.Scheduler;
 import io.deephaven.util.SafeCloseable;
+import io.grpc.Context;
 import io.grpc.stub.ServerCallStreamObserver;
 import io.grpc.stub.StreamObserver;
 import org.jetbrains.annotations.NotNull;
@@ -188,7 +189,7 @@ public class ConsoleServiceGrpcImpl extends ConsoleServiceGrpc.ConsoleServiceImp
                     .requiresSerialQueue()
                     .require(exportedConsole)
                     .onError(responseObserver)
-                    .submit(() -> {
+                    .submit(Context.current().wrap(() -> {
                         ScriptSession scriptSession = exportedConsole.get();
                         ScriptSession.Changes changes = scriptSession.evaluateScript(request.getCode());
                         ExecuteCommandResponse.Builder diff = ExecuteCommandResponse.newBuilder();
@@ -204,7 +205,7 @@ public class ConsoleServiceGrpcImpl extends ConsoleServiceGrpc.ConsoleServiceImp
                             log.error().append("Error running script: ").append(changes.error).endl();
                         }
                         safelyComplete(responseObserver, diff.setChanges(fieldChanges).build());
-                    });
+                    }));
         }
     }

which would then allow access to io.deephaven.server.session.SessionServiceGrpcImpl#SESSION_CONTEXT_KEY. If this is deemed unacceptable for security reasons, a tighter implementation of ExportUtil.getExport could be developed.

Another possibility would be to update ExecuteCommand to take a tickets as input, and have some canonical way to reference them from the script:

message ExecuteCommandRequest {
    io.deephaven.proto.backplane.grpc.Ticket console_id = 1;
    reserved 2;//if script sessions get a ticket, we will use this reserved tag
    string code = 3;
    repeated io.deephaven.proto.backplane.grpc.Ticket inputs = 4;
}
myResultingTable = SomeCoolOperation.create(ExecuteCommandUtil.getInput(0))
rcaudy commented 2 months ago

ExportUtil.getExport seems dangerous if you race your execute with an incomplete RPC that exports something under a client-assigned ID. You could potentially block the RPC from completing, since command execution holds the exclusive lock, and thereby deadlock the update graph. I think getExport would need to be careful to either use an update graph condition, or only resolve completed exports (throwing an error otherwise).