Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Clean up how objects are managed when they are fetched #4909

Closed
wants to merge 20 commits into from
Closed
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
fix: Handle exported objects fetches more correctly
- Previously, exported objects were not handling their fetched objects correctly
  - Tables were not using copies, only widgets were
  - The parent ticket was not being released in many cases (TreeTable, PartitonedTable, PandasTable)
- Refactored so exported objects always follow the same path
  - Just call connection.getObject, which always creates a new ticket for the object
  - Still need to close the widget so it closes the exported objects
- Test by adding debug logging for SessionState:
```py
import jpy
logger = jpy.get_type('org.slf4j.LoggerFactory').getLogger('io.deephaven.server.session.SessionState')
jpy.cast(logger, jpy.get_type('ch.qos.logback.classic.Logger')).setLevel(jpy.get_type('ch.qos.logback.classic.Level').DEBUG)
```
mofojed committed Dec 1, 2023
commit f48b18f8515afca4d3f17880d41c9e2c00bea246
Original file line number Diff line number Diff line change
@@ -108,7 +108,14 @@ public Promise<JsPartitionedTable> refetch() {
this.columns = JsObject.freeze(columns);
this.keyColumns = JsObject.freeze(keyColumns);

return w.getExportedObjects()[0].fetch();

Promise<?> promise = w.getExportedObjects()[0].fetch();
promise.then(ignore -> {
// We only need to keep the widget open long enough to get the exported objects
w.close();
return null;
});
return promise;
}).then(result -> {
keys = (JsTable) result;
// TODO(deephaven-core#3604) in case of a new session, we should do a full refetch
@@ -308,6 +315,8 @@ public Promise<JsTable> getKeyTable() {
* will not affect tables in use.
*/
public void close() {
widget.close();

if (keys != null) {
keys.close();
}
Original file line number Diff line number Diff line change
@@ -759,14 +759,27 @@ public Promise<JsTable> getTable(JsVariableDefinition varDef, @Nullable Boolean
});
}

public Promise<?> getObject(TypedTicket typedTicket) {
return getObject(
new JsVariableDefinition(typedTicket.getType(), null, typedTicket.getTicket().getTicket_asB64(), null));
}

public Promise<?> getObject(JsVariableDefinition definition) {
if (JsVariableType.TABLE.equalsIgnoreCase(definition.getType())) {
return getTable(definition, null);
} else if (JsVariableType.FIGURE.equalsIgnoreCase(definition.getType())) {
return getFigure(definition);
} else if (JsVariableType.PANDAS.equalsIgnoreCase(definition.getType())) {
return getWidget(definition)
.then(widget -> widget.getExportedObjects()[0].fetch());
.then(widget -> {
Promise<?> promise = widget.getExportedObjects()[0].fetch();
promise.then(ignore -> {
// We only need to keep the widget open long enough to get the exported objects
widget.close();
return null;
});
return promise;
});
} else if (JsVariableType.PARTITIONEDTABLE.equalsIgnoreCase(definition.getType())) {
return getPartitionedTable(definition);
} else if (JsVariableType.HIERARCHICALTABLE.equalsIgnoreCase(definition.getType())) {
Original file line number Diff line number Diff line change
@@ -979,8 +979,7 @@ public void close() {

connection.unregisterSimpleReconnectable(this);

// Presently it is never necessary to release widget tickets, since they can't be export tickets.
// connection.releaseTicket(widget.getTicket());
connection.releaseTicket(widget.getTicket());
mofojed marked this conversation as resolved.
Show resolved Hide resolved

if (filteredTable != null) {
filteredTable.release();
Original file line number Diff line number Diff line change
@@ -6,15 +6,10 @@
import com.vertispan.tsdefs.annotations.TsInterface;
import com.vertispan.tsdefs.annotations.TsName;
import elemental2.promise.Promise;
import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.table_pb.ExportedTableCreationResponse;
import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.ticket_pb.TypedTicket;
import io.deephaven.web.client.api.Callbacks;
import io.deephaven.web.client.api.JsTable;
import io.deephaven.web.client.api.ServerObject;
import io.deephaven.web.client.api.WorkerConnection;
import io.deephaven.web.client.api.console.JsVariableDefinition;
import io.deephaven.web.client.api.console.JsVariableType;
import io.deephaven.web.client.state.ClientTableState;
import jsinterop.annotations.JsMethod;
import jsinterop.annotations.JsProperty;

@@ -47,24 +42,15 @@ public TypedTicket typedTicket() {
return typedTicket;
}

/**
* Fetches the object from the server. Note that the returned object has it's own ticket and must be closed, and you
* must call {@link #close()} on this object as well whether you fetch the exported object or not.
*
mofojed marked this conversation as resolved.
Show resolved Hide resolved
* @return a promise that resolves to the object
*/
@JsMethod
public Promise<?> fetch() {
if (getType().equals(JsVariableType.TABLE)) {
return Callbacks.<ExportedTableCreationResponse, Object>grpcUnaryPromise(c -> {
connection.tableServiceClient().getExportedTableCreationResponse(ticket.getTicket(),
connection.metadata(),
c::apply);
}).then(etcr -> {
ClientTableState cts = connection.newStateFromUnsolicitedTable(etcr, "table for widget");
JsTable table = new JsTable(connection, cts);
// never attempt a reconnect, since we might have a different widget schema entirely
table.addEventListener(JsTable.EVENT_DISCONNECT, ignore -> table.close());
return Promise.resolve(table);
});
} else {
return this.connection.getObject(
new JsVariableDefinition(ticket.getType(), null, ticket.getTicket().getTicket_asB64(), null));
}
return this.connection.getObject(ticket);
}

/**