Skip to content

Commit

Permalink
Fix JS API reconnecting with failed tables (#5501)
Browse files Browse the repository at this point in the history
DHC reconnects are able to restore server streams to an existing session, but
most of the JS API was written to assume that a lost connection requires
rebuilding objects on the server by replaying operations. This fix handles
cases where a table failed and then a network error occurred, causing the
table to be stuck unable to reconnect, since the table has failed.

Two bugs prevented this from working, both cases where after some operation
couldn't be scheduled, a microtask would immediately try again, leading
effectively to an infinite loop in the browser. Table subscriptions are fixed
by first checking if the table is running, so can be subscribed, and table
refetch is fixed by using null for its fetcher, and during refetch if the
fetcher is null either fail right away with the existing fail message, or
succeed right away.

This fix currently makes it possible for a failed table on a reconnected worker
to not signal that it is still failed - this will be addressed in a follow-up.

Fixes #5414
  • Loading branch information
niloc132 authored and stanbrub committed May 17, 2024
1 parent 59de583 commit 58c8337
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 13 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -1399,7 +1399,7 @@ private void flush() {
}
}
}
} else {
} else if (state.isRunning()) {
List<TableSubscriptionRequest> vps = new ArrayList<>();
state.forActiveSubscriptions((table, subscription) -> {
assert table.isActive(state) : "Inactive table has a viewport still attached";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
import io.deephaven.web.client.api.barrage.WebBarrageUtils;
import io.deephaven.web.client.api.barrage.def.ColumnDefinition;
import io.deephaven.web.client.api.barrage.def.InitialTableDefinition;
import io.deephaven.web.client.api.barrage.def.TableAttributesDefinition;
import io.deephaven.web.client.api.batch.TableConfig;
import io.deephaven.web.client.api.filter.FilterCondition;
import io.deephaven.web.client.api.lifecycle.HasLifecycle;
Expand All @@ -28,12 +27,8 @@
import jsinterop.base.Js;

import java.util.*;
import java.util.function.BinaryOperator;
import java.util.function.Function;
import java.util.stream.Collector;
import java.util.stream.Collectors;

import static io.deephaven.web.client.api.barrage.WebBarrageUtils.keyValuePairs;
import static io.deephaven.web.client.fu.JsItr.iterate;

/**
Expand Down Expand Up @@ -251,13 +246,7 @@ public ClientTableState newState(TableTicket newHandle, TableConfig config) {

final ClientTableState newState = new ClientTableState(
connection, newHandle, sorts, conditions, filters, customColumns, dropColumns, viewColumns, this,
(c, s, metadata) -> {
// This fetcher will not be used for the initial fetch, only for refetches.
// Importantly, any CTS with a source (what we are creating here; source=this, above)
// is revived, it does not use the refetcher; we directly rebuild batch operations instead.
// It may make sense to actually have batches route through reviver instead.
connection.getReviver().revive(metadata, s);
}, config.toSummaryString());
null, config.toSummaryString());
newState.setFlat(config.isFlat());
if (!isRunning()) {
onFailed(reason -> newState.setResolution(ResolutionState.FAILED, reason), JsRunnable.doNothing());
Expand Down Expand Up @@ -991,6 +980,12 @@ public Promise<JsTable> fetchTable(HasEventHandling failHandler, BrowserHeaders
}

public Promise<ClientTableState> refetch(HasEventHandling failHandler, BrowserHeaders metadata) {
if (fetch == null) {
if (failMsg != null) {
return Promise.reject(failMsg);
}
return Promise.resolve(this);
}
final Promise<ExportedTableCreationResponse> promise =
Callbacks.grpcUnaryPromise(c -> fetch.fetch(c, this, metadata));
// noinspection unchecked
Expand Down

0 comments on commit 58c8337

Please sign in to comment.