Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
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
1 change: 1 addition & 0 deletions CHANGELOG.unreleased.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ For upgrade instructions, please check the [migration guide](MIGRATIONS.released
### Fixed
- Fixed a bug that would lock a non existing mapping to an empty segmentation layer under certain conditions. [#8401](https://github.com/scalableminds/webknossos/pull/8401)
- Fixed the alignment of the button that allows restricting floodfill operations to a bounding box. [#8388](https://github.com/scalableminds/webknossos/pull/8388)
- Fixed rare bug where saving got stuck. [#8409](https://github.com/scalableminds/webknossos/pull/8409)

### Removed

Expand Down
2 changes: 1 addition & 1 deletion frontend/javascripts/oxalis/model/actions/save_actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ export type SaveAction =

// The action creators pushSaveQueueTransaction and pushSaveQueueTransactionIsolated
// are typed so that update actions that need isolation are isolated in a group each.
// From this point on, we can assume that the groups fulfil the isolation requirement.
// From this point on, we can assume that the groups fulfill the isolation requirement.
export const pushSaveQueueTransaction = (
items: Array<UpdateActionWithoutIsolationRequirement>,
): PushSaveQueueTransaction =>
Expand Down
2 changes: 1 addition & 1 deletion frontend/javascripts/oxalis/model/reducers/save_reducer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ function SaveReducer(state: OxalisState, action: Action): OxalisState {
}

case "SET_LAST_SAVE_TIMESTAMP": {
return updateKey2(state, "save", "lastSaveTimestamp", action.timestamp);
return updateKey(state, "save", { lastSaveTimestamp: action.timestamp });
}

case "SET_VERSION_NUMBER": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ function shouldDisplaySegmentationData(): boolean {
return !onlyViewing3dViewport;
}

export function* warnAboutSegmentationZoom(): Saga<void> {
export function* warnAboutSegmentationZoom(): Saga<never> {
function* warnMaybe(): Saga<void> {
const segmentationLayer = Model.getVisibleSegmentationLayer();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { select } from "oxalis/model/sagas/effect-generators";
import { call, put, take } from "typed-redux-saga";
import { ensureWkReady } from "./ready_sagas";

export function* watchToolDeselection(): Saga<void> {
export function* watchToolDeselection(): Saga<never> {
yield* call(ensureWkReady);
let previousTool = yield* select((state) => state.uiInformation.activeTool);

Expand All @@ -38,7 +38,7 @@ export function* watchToolDeselection(): Saga<void> {
}
}

export function* watchToolReset(): Saga<void> {
export function* watchToolReset(): Saga<never> {
while (true) {
yield* take("ESCAPE");
const activeTool = yield* select((state) => state.uiInformation.activeTool);
Expand Down
2 changes: 1 addition & 1 deletion frontend/javascripts/oxalis/model/sagas/mapping_saga.ts
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ function createBucketRetrievalSourceChannel(layerName: string) {
}, buffers.sliding<BucketRetrievalSource>(1));
}

function* watchChangedBucketsForLayer(layerName: string): Saga<void> {
function* watchChangedBucketsForLayer(layerName: string): Saga<never> {
const dataCube = yield* call([Model, Model.getCubeByLayerName], layerName);
const bucketChannel = yield* call(createBucketDataChangedChannel, dataCube);

Expand Down
8 changes: 5 additions & 3 deletions frontend/javascripts/oxalis/model/sagas/mesh_saga.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1199,7 +1199,7 @@ function* handleMeshVisibilityChange(action: UpdateMeshVisibilityAction): Saga<v
segmentMeshController.setMeshVisibility(id, visibility, layerName, additionalCoordinates);
}

export function* handleAdditionalCoordinateUpdate(): Saga<void> {
export function* handleAdditionalCoordinateUpdate(): Saga<never> {
// We want to prevent iterating through all additional coordinates to adjust the mesh visibility, so we store the
// previous additional coordinates in this method. Thus we have to catch SET_ADDITIONAL_COORDINATES actions in a
// while-true loop and register this saga in the root saga instead of calling from the mesh saga.
Expand All @@ -1212,11 +1212,13 @@ export function* handleAdditionalCoordinateUpdate(): Saga<void> {
const action = (yield* take(["SET_ADDITIONAL_COORDINATES"]) as any) as FlycamAction;
// Satisfy TS
if (action.type !== "SET_ADDITIONAL_COORDINATES") {
throw new Error("Unexpected action type");
// Don't throw as this would interfere with the never return type
console.error("Unexpected action.type. Ignoring SET_ADDITIONAL_COORDINATES action...");
continue;
}
const meshRecords = segmentMeshController.meshesGroupsPerSegmentId;

if (action.values == null || action.values.length === 0) break;
if (action.values == null || action.values.length === 0) continue;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Changed break to continue when values are empty

This is a crucial fix that likely addresses the core issue. Previously, the saga would exit with break when action.values was null or empty, which would terminate the saga permanently. Changing to continue ensures the saga keeps listening for subsequent actions, maintaining its long-running nature.

Comment on lines +1217 to +1221
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this ok to change? The previously thrown error is now swallowed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, the handling is only done for TS, as far as I understand. I can output a warning to console, though.

const newAdditionalCoordKey = getAdditionalCoordinatesAsString(action.values);

for (const additionalCoordinates of [action.values, previousAdditionalCoordinates]) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,10 @@ function* getMask(
];
}

function* showApproximatelyProgress(amount: number, expectedDurationPerItemMs: number) {
function* showApproximatelyProgress(
amount: number,
expectedDurationPerItemMs: number,
): Saga<never> {
// The progress bar is split into amount + 1 chunks. The first amount
// chunks are filled after expectedDurationPerItemMs passed.
// Afterwards, only one chunk is missing. With each additional iteration,
Expand Down
2 changes: 1 addition & 1 deletion frontend/javascripts/oxalis/model/sagas/root_saga.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import { setIsWkReadyAction } from "../actions/ui_actions";
import { warnIfEmailIsUnverified } from "./user_saga";

let rootSagaCrashed = false;
export default function* rootSaga(): Saga<void> {
export default function* rootSaga(): Saga<never> {
while (true) {
rootSagaCrashed = false;
const task = yield* fork(restartableSaga);
Expand Down
21 changes: 12 additions & 9 deletions frontend/javascripts/oxalis/model/sagas/save_saga.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import Toast from "libs/toast";
import { sleep } from "libs/utils";
import window, { alert, document, location } from "libs/window";
import _ from "lodash";
import memoizeOne from "memoize-one";
import messages from "messages";
import { ControlModeEnum } from "oxalis/constants";
import { getMagInfo } from "oxalis/model/accessors/dataset_accessor";
Expand Down Expand Up @@ -57,7 +58,7 @@ import { takeEveryWithBatchActionSupport } from "./saga_helpers";

const ONE_YEAR_MS = 365 * 24 * 3600 * 1000;

export function* pushSaveQueueAsync(): Saga<void> {
export function* pushSaveQueueAsync(): Saga<never> {
yield* call(ensureWkReady);

yield* put(setLastSaveTimestampAction());
Expand Down Expand Up @@ -87,7 +88,7 @@ export function* pushSaveQueueAsync(): Saga<void> {
});
yield* put(setSaveBusyAction(true));

// Send (parts) of the save queue to the server.
// Send (parts of) the save queue to the server.
// There are two main cases:
// 1) forcePush is true
// The user explicitly requested to save an annotation.
Expand All @@ -100,7 +101,7 @@ export function* pushSaveQueueAsync(): Saga<void> {
// The auto-save interval was reached at time T. The following code
// will determine how many items are in the save queue at this time T.
// Exactly that many items will be sent to the server.
// New items that might be added to the save queue during saving, will
// New items that might be added to the save queue during saving, will be
// ignored (they will be picked up in the next iteration of this loop).
// Otherwise, the risk of a high number of save-requests (see case 1)
// would be present here, too (note the risk would be greater, because the
Expand All @@ -118,7 +119,6 @@ export function* pushSaveQueueAsync(): Saga<void> {
break;
}
}

yield* put(setSaveBusyAction(false));
}
}
Expand Down Expand Up @@ -183,7 +183,6 @@ export function* sendSaveRequestToServer(): Saga<number> {
const startTime = Date.now();
yield* call(
sendRequestWithToken,

`${tracingStoreUrl}/tracings/annotation/${annotationId}/update?token=`,
{
method: "POST",
Expand Down Expand Up @@ -281,12 +280,16 @@ export function* sendSaveRequestToServer(): Saga<number> {
}

function* markBucketsAsNotDirty(saveQueue: Array<SaveQueueEntry>) {
const getLayerAndMagInfoForTracingId = memoizeOne((tracingId: string) => {
const segmentationLayer = Model.getSegmentationTracingLayer(tracingId);
const segmentationMagInfo = getMagInfo(segmentationLayer.mags);
return [segmentationLayer, segmentationMagInfo] as const;
});
Comment on lines 282 to +287
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Added memoization to optimize bucket processing

Great performance optimization! This directly addresses the PR objective by reducing redundant calculations in the markBucketsAsNotDirty function, which was causing the application to become unresponsive when processing a large number of buckets at once.

The memoization ensures that for a given tracingId, the layer and magInfo are only computed once, which can significantly improve performance when processing thousands of buckets.

for (const saveEntry of saveQueue) {
for (const updateAction of saveEntry.actions) {
if (updateAction.name === "updateBucket") {
const { actionTracingId: tracingId } = updateAction.value;
const segmentationLayer = Model.getSegmentationTracingLayer(tracingId);
const segmentationMagInfo = yield* call(getMagInfo, segmentationLayer.mags);
const [segmentationLayer, segmentationMagInfo] = getLayerAndMagInfoForTracingId(tracingId);
Comment on lines -288 to +292
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the fix, correct?

Strange that non-nested yield calls can lead to a stack overflow :/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, apparently, the effects are processed tail recursively by redux which leads to this problem.


const { position, mag, additionalCoordinates } = updateAction.value;
const magIndex = segmentationMagInfo.getIndexByMag(mag);
Expand Down Expand Up @@ -377,7 +380,7 @@ export function* saveTracingAsync(): Saga<void> {

export function* setupSavingForTracingType(
initializeAction: InitializeSkeletonTracingAction | InitializeVolumeTracingAction,
): Saga<void> {
): Saga<never> {
/*
Listen to changes to the annotation and derive UpdateActions from the
old and new state.
Expand Down Expand Up @@ -450,7 +453,7 @@ const VERSION_POLL_INTERVAL_COLLAB = 10 * 1000;
const VERSION_POLL_INTERVAL_READ_ONLY = 60 * 1000;
const VERSION_POLL_INTERVAL_SINGLE_EDITOR = 30 * 1000;

function* watchForSaveConflicts() {
function* watchForSaveConflicts(): Saga<never> {
function* checkForNewVersion() {
const allowSave = yield* select(
(state) => state.tracing.restrictions.allowSave && state.tracing.restrictions.allowUpdate,
Expand Down
2 changes: 1 addition & 1 deletion frontend/javascripts/oxalis/throttled_store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ Store.subscribe(() => {
}
});

async function go() {
async function go(): Promise<never> {
while (true) {
await waitForUpdate.promise();
waitForUpdate = new Deferred();
Expand Down