Skip to content

Commit

Permalink
Extract variable api from ScriptSession, let ScriptSession guard reads
Browse files Browse the repository at this point in the history 
This patch moves from assuming that the ScriptSession owns the
ExecContext for locking (with UpdateGraph) and variable access (with
QueryScope).

Previously, the groovy script session used a lock to interact with its
query scope, but python did not under the incorrect assumption that the
GIL would guard those reads. Instead, a read/write lock is used to
ensure that reads block while an operation is running that could affect
values, but avoiding reads blocking each other.

Partial deephaven#4040
  • Loading branch information
@niloc132
niloc132 committed Dec 20, 2023
1 parent 6d3702b commit 05fce0f
Show file tree
Hide file tree
Showing 15 changed files with 209 additions and 246 deletions.
8 changes: 0 additions & 8 deletions Integrations/src/main/java/io/deephaven/integrations/python/PythonDeephavenSession.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import io.deephaven.plugin.type.ObjectTypeLookup.NoOp;
import io.deephaven.util.SafeCloseable;
import io.deephaven.util.annotations.ScriptApi;
import io.deephaven.util.annotations.VisibleForTesting;
import io.deephaven.util.thread.NamingThreadFactory;
import io.deephaven.util.thread.ThreadInitializationFactory;
import org.jetbrains.annotations.NotNull;
Expand Down Expand Up @@ -144,13 +143,6 @@ public Thread newThread(@NotNull Runnable r) {
}
}

@Override
@VisibleForTesting
public QueryScope newQueryScope() {
// depend on the GIL instead of local synchronization
return new UnsynchronizedScriptSessionQueryScope(this);
}

/**
* Finds the specified script; and runs it as a file, or if it is a stream writes it to a temporary file in order to
* run it.
Expand Down
15 changes: 6 additions & 9 deletions engine/sql/src/main/java/io/deephaven/engine/sql/Sql.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,11 @@
import io.deephaven.base.log.LogOutput;
import io.deephaven.base.log.LogOutput.ObjFormatter;
import io.deephaven.engine.context.ExecutionContext;
import io.deephaven.engine.context.QueryScope;
import io.deephaven.engine.table.ColumnDefinition;
import io.deephaven.engine.table.Table;
import io.deephaven.engine.table.TableDefinition;
import io.deephaven.engine.table.impl.TableCreatorImpl;
import io.deephaven.engine.util.AbstractScriptSession.ScriptSessionQueryScope;
import io.deephaven.engine.util.ScriptSession;
import io.deephaven.internal.log.LoggerFactory;
import io.deephaven.io.logger.Logger;
import io.deephaven.qst.column.header.ColumnHeader;
Expand Down Expand Up @@ -83,18 +82,16 @@ private static Map<String, Table> currentScriptSessionNamedTables() {
final Map<String, Table> scope = new HashMap<>();
// getVariables() is inefficient
// See SQLTODO(catalog-reader-implementation)
for (Entry<String, Object> e : currentScriptSession().getVariables().entrySet()) {
if (e.getValue() instanceof Table) {
scope.put(e.getKey(), (Table) e.getValue());
QueryScope queryScope = ExecutionContext.getContext().getQueryScope();
for (String name : queryScope.getParamNames()) {
Object paramValue = queryScope.readParamValue(name);
if (paramValue instanceof Table) {
scope.put(name, (Table) paramValue);
}
}
return scope;
}

private static ScriptSession currentScriptSession() {
return ((ScriptSessionQueryScope) ExecutionContext.getContext().getQueryScope()).scriptSession();
}

private static TableHeader adapt(TableDefinition tableDef) {
final Builder builder = TableHeader.builder();
for (ColumnDefinition<?> cd : tableDef.getColumns()) {
Expand Down
200 changes: 137 additions & 63 deletions engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,12 @@
import java.io.IOException;
import java.nio.file.Path;
import java.util.LinkedHashSet;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.UUID;
import java.util.concurrent.locks.ReadWriteLock;
import java.util.concurrent.locks.ReentrantReadWriteLock;

import static io.deephaven.engine.table.Table.NON_DISPLAY_TABLE;

Expand All @@ -42,7 +45,7 @@
* evaluateScript which handles liveness and diffs in a consistent way.
*/
public abstract class AbstractScriptSession<S extends AbstractScriptSession.Snapshot> extends LivenessScope
implements ScriptSession, VariableProvider {
implements ScriptSession {

private static final Path CLASS_CACHE_LOCATION = CacheDir.get().resolve("script-session-classes");

Expand All @@ -68,6 +71,8 @@ private static void createOrClearDirectory(final File directory) {
private final ObjectTypeLookup objectTypeLookup;
private final Listener changeListener;

private final ReadWriteLock variableAccessLock = new ReentrantReadWriteLock();

private S lastSnapshot;

protected AbstractScriptSession(
Expand Down Expand Up @@ -141,6 +146,7 @@ public synchronized final Changes evaluateScript(final String script, @Nullable
final Changes diff;
// retain any objects which are created in the executed code, we'll release them when the script session
// closes
variableAccessLock.writeLock().lock();
try (final S initialSnapshot = takeSnapshot();
final SafeCloseable ignored = LivenessScopeStack.open(this, false)) {

Expand All @@ -157,6 +163,8 @@ public synchronized final Changes evaluateScript(final String script, @Nullable
observeScopeChanges();
// Use the "last" snapshot created as a side effect of observeScopeChanges() as our "to"
diff = createDiff(initialSnapshot, lastSnapshot, evaluateErr);
} finally {
variableAccessLock.writeLock().unlock();
}

return diff;
Expand Down Expand Up @@ -247,10 +255,11 @@ protected void destroy() {
/**
* @return a query scope for this session; only invoked during construction
*/
protected abstract QueryScope newQueryScope();
protected QueryScope newQueryScope() {
return new ScriptSessionQueryScope();
}

@Override
public Class<?> getVariableType(final String var) {
private Class<?> getVariableType(final String var) {
final Object result = getVariable(var, null);
if (result == null) {
return null;
Expand All @@ -262,47 +271,149 @@ public Class<?> getVariableType(final String var) {
}


@Override
public TableDefinition getTableDefinition(final String var) {
Object o = getVariable(var, null);
return o instanceof Table ? ((Table) o).getDefinition() : null;
}


/**
* Retrieve a variable from the script session's bindings.
* <p/>
* Please use {@link ScriptSession#getVariable(String, Object)} if you expect the variable may not exist.
*
* @param name the variable to retrieve
* @return the variable
* @throws QueryScope.MissingVariableException if the variable does not exist
*/
@NotNull
protected abstract Object getVariable(String name) throws QueryScope.MissingVariableException;

/**
* Retrieve a variable from the script session's bindings. If the variable is not present, return defaultValue.
* <p>
* If the variable is present, but is not of type (T), a ClassCastException may result.
*
* @param name the variable to retrieve
* @param defaultValue the value to use when no value is present in the session's scope
* @param <T> the type of the variable
* @return the value of the variable, or defaultValue if not present
*/
protected abstract <T> T getVariable(String name, T defaultValue);

/**
* Retrieves all of the variables present in the session's scope (e.g., Groovy binding, Python globals()).
*
* @return an unmodifiable map with variable names as the keys, and the Objects as the result
*/
protected abstract Map<String, Object> getVariables();

/**
* Retrieves all of the variable names present in the session's scope
*
* @return an unmodifiable set of variable names
*/
protected abstract Set<String> getVariableNames();

/**
* Check if the scope has the given variable name
*
* @param name the variable name
* @return True iff the scope has the given variable name
*/
protected abstract boolean hasVariableName(String name);

/**
* Inserts a value into the script's scope.
*
* @param name the variable name to set
* @param value the new value of the variable
*/
protected abstract void setVariable(String name, @Nullable Object value);

@Override
public VariableProvider getVariableProvider() {
return this;
return new VariableProvider() {
@Override
public Set<String> getVariableNames() {
variableAccessLock.readLock().lock();
try {
return AbstractScriptSession.this.getVariableNames();
} finally {
variableAccessLock.readLock().unlock();
}
}

@Override
public Class<?> getVariableType(String var) {
variableAccessLock.readLock().lock();
try {
return AbstractScriptSession.this.getVariableType(var);
} finally {
variableAccessLock.readLock().unlock();
}
}

@Override
public <T> T getVariable(String var, T defaultValue) {
variableAccessLock.readLock().lock();
try {
return AbstractScriptSession.this.getVariable(var, defaultValue);
} finally {
variableAccessLock.readLock().unlock();
}
}

@Override
public TableDefinition getTableDefinition(String var) {
variableAccessLock.readLock().lock();
try {
return AbstractScriptSession.this.getTableDefinition(var);
} finally {
variableAccessLock.readLock().unlock();
}
}

@Override
public boolean hasVariableName(String name) {
variableAccessLock.readLock().lock();
try {
return AbstractScriptSession.this.hasVariableName(name);
} finally {
variableAccessLock.readLock().unlock();
}
}

@Override
public <T> void setVariable(String name, T value) {
variableAccessLock.writeLock().lock();
try {
AbstractScriptSession.this.setVariable(name, value);
} finally {
variableAccessLock.writeLock().unlock();
}
}
};
}

// -----------------------------------------------------------------------------------------------------------------
// ScriptSession-based QueryScope implementation, with no remote scope or object reflection support
// -----------------------------------------------------------------------------------------------------------------

public abstract static class ScriptSessionQueryScope extends QueryScope {
final ScriptSession scriptSession;

public ScriptSessionQueryScope(ScriptSession scriptSession) {
this.scriptSession = scriptSession;
}

public class ScriptSessionQueryScope extends QueryScope {
@Override
public void putObjectFields(Object object) {
throw new UnsupportedOperationException();
}

public ScriptSession scriptSession() {
return scriptSession;
}
}

public static class UnsynchronizedScriptSessionQueryScope extends ScriptSessionQueryScope {
public UnsynchronizedScriptSessionQueryScope(@NotNull final ScriptSession scriptSession) {
super(scriptSession);
return AbstractScriptSession.this;
}

@Override
public Set<String> getParamNames() {
final Set<String> result = new LinkedHashSet<>();
for (final String name : scriptSession.getVariableNames()) {
for (final String name : getVariableProvider().getVariableNames()) {
if (NameValidator.isValidQueryParameterName(name)) {
result.add(name);
}
Expand All @@ -312,7 +423,7 @@ public Set<String> getParamNames() {

@Override
public boolean hasParamName(String name) {
return NameValidator.isValidQueryParameterName(name) && scriptSession.hasVariableName(name);
return NameValidator.isValidQueryParameterName(name) && getVariableProvider().hasVariableName(name);
}

@Override
Expand All @@ -322,7 +433,7 @@ protected <T> QueryScopeParam<T> createParam(final String name)
throw new QueryScope.MissingVariableException("Name " + name + " is invalid");
}
// noinspection unchecked
return new QueryScopeParam<>(name, (T) scriptSession.getVariable(name));
return new QueryScopeParam<>(name, (T) getVariableProvider().getVariable(name, null));
}

@Override
Expand All @@ -331,57 +442,20 @@ public <T> T readParamValue(final String name) throws QueryScope.MissingVariable
throw new QueryScope.MissingVariableException("Name " + name + " is invalid");
}
// noinspection unchecked
return (T) scriptSession.getVariable(name);
return (T) getVariableProvider().getVariable(name, null);
}

@Override
public <T> T readParamValue(final String name, final T defaultValue) {
if (!NameValidator.isValidQueryParameterName(name)) {
return defaultValue;
}
return scriptSession.getVariable(name, defaultValue);
return getVariableProvider().getVariable(name, defaultValue);
}

@Override
public <T> void putParam(final String name, final T value) {
scriptSession.setVariable(NameValidator.validateQueryParameterName(name), value);
}
}

public static class SynchronizedScriptSessionQueryScope extends UnsynchronizedScriptSessionQueryScope {
public SynchronizedScriptSessionQueryScope(@NotNull final ScriptSession scriptSession) {
super(scriptSession);
}

@Override
public synchronized Set<String> getParamNames() {
return super.getParamNames();
}

@Override
public synchronized boolean hasParamName(String name) {
return super.hasParamName(name);
}

@Override
protected synchronized <T> QueryScopeParam<T> createParam(final String name)
throws QueryScope.MissingVariableException {
return super.createParam(name);
}

@Override
public synchronized <T> T readParamValue(final String name) throws QueryScope.MissingVariableException {
return super.readParamValue(name);
}

@Override
public synchronized <T> T readParamValue(final String name, final T defaultValue) {
return super.readParamValue(name, defaultValue);
}

@Override
public synchronized <T> void putParam(final String name, final T value) {
super.putParam(name, value);
getVariableProvider().setVariable(NameValidator.validateQueryParameterName(name), value);
}
}
}
Loading

0 comments on commit 05fce0f

Please sign in to comment.