Skip to content

Commit

Permalink
fix: JS clients must not release tickets for pending operations
Browse files Browse the repository at this point in the history
When a necessary operation is still running, clients must not release
upstream tickets. This has mostly worked by the server running fast
enough or by the websocket transport serializing operations in a way
that we can't always rely on.

Testing this is difficult to reliably do - the ticket has some manual
steps that generally make it possible to see the issue, and we're
exploring other options to make bugs like this more obvious. At this
time, there is no good way to put an integration test in that verifies
correct behavior efficiently.

Fixes deephaven#6056
Fixes DH-18486
  • Loading branch information
niloc132 committed Jan 23, 2025
1 parent e7f731b commit f6b0241
Showing 1 changed file with 32 additions and 21 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ private enum RebuildStep {
private Object[][] keyTableData;
private Promise<JsTable> keyTable;

private TicketAndPromise<?> viewTicket;
private TicketAndPromise<ClientTableState> viewTicket;
private Promise<TreeSubscription> stream;

// the "next" set of filters/sorts that we'll use. these either are "==" to the above fields, or are scheduled
Expand Down Expand Up @@ -348,7 +348,7 @@ private Promise<JsTable> makeKeyTable() {
return keyTable;
}

private TicketAndPromise<?> makeView(TicketAndPromise<?> prevTicket) {
private TicketAndPromise<ClientTableState> makeView(TicketAndPromise<?> prevTicket) {
if (viewTicket != null) {
return viewTicket;
}
Expand All @@ -371,7 +371,30 @@ private TicketAndPromise<?> makeView(TicketAndPromise<?> prevTicket) {
c.apply(error, null);
return null;
});
}), connection);
}).then(result -> {
ClientTableState state = new ClientTableState(connection,
new TableTicket(viewTicket.ticket().getTicket_asU8()), (callback, newState, metadata) -> {
callback.apply("fail, trees dont reconnect like this", null);
}, "");
state.retain(JsTreeTable.this);
ExportedTableCreationResponse def = new ExportedTableCreationResponse();
HierarchicalTableDescriptor treeDescriptor =
HierarchicalTableDescriptor.deserializeBinary(widget.getDataAsU8());
def.setSchemaHeader(treeDescriptor.getSnapshotSchema_asU8());
def.setResultId(new TableReference());
def.getResultId().setTicket(viewTicket.ticket());
state.applyTableCreationResponse(def);
return Promise.resolve(state);
}), connection) {
@Override
public void release() {
super.release();
then(state -> {
state.unretain(JsTreeTable.this);
return null;
});
}
};
return viewTicket;
}

Expand Down Expand Up @@ -622,16 +645,16 @@ private void replaceSubscription(RebuildStep step) {
Promise<TreeSubscription> stream = Promise.resolve(defer())
.then(ignore -> {
makeKeyTable();
TicketAndPromise filter = prepareFilter();
TicketAndPromise sort = prepareSort(filter);
TicketAndPromise view = makeView(sort);
TicketAndPromise<?> filter = prepareFilter();
TicketAndPromise<?> sort = prepareSort(filter);
TicketAndPromise<ClientTableState> view = makeView(sort);
return Promise.all(
keyTable,
filter.promise(),
sort.promise(),
view.promise());
sort.promise())
.then(others -> view.promise());
})
.then(results -> {
.then(state -> {
BitSet columnsBitset = makeColumnSubscriptionBitset();
RangeSet range = RangeSet.ofRange((long) (double) firstRow, (long) (double) lastRow);

Expand All @@ -644,18 +667,6 @@ private void replaceSubscription(RebuildStep step) {
range,
alwaysFireEvent);

ClientTableState state = new ClientTableState(connection,
new TableTicket(viewTicket.ticket().getTicket_asU8()), (callback, newState, metadata) -> {
callback.apply("fail, trees dont reconnect like this", null);
}, "");
ExportedTableCreationResponse def = new ExportedTableCreationResponse();
HierarchicalTableDescriptor treeDescriptor =
HierarchicalTableDescriptor.deserializeBinary(widget.getDataAsU8());
def.setSchemaHeader(treeDescriptor.getSnapshotSchema_asU8());
def.setResultId(new TableReference());
def.getResultId().setTicket(viewTicket.ticket());
state.applyTableCreationResponse(def);

TreeSubscription subscription = new TreeSubscription(state, connection);

subscription.addEventListener(TreeSubscription.EVENT_UPDATED,
Expand Down

0 comments on commit f6b0241

Please sign in to comment.