From a0caa98c903c19bf4307fe436bb1038abe40f20a Mon Sep 17 00:00:00 2001 From: Bill Wallace Date: Tue, 9 Jan 2024 18:53:23 -0500 Subject: [PATCH 1/6] fix: Lockup in request pool when returning non promise result --- common/reviews/api/tools.api.md | 4 ++++ .../src/requestPool/requestPoolManager.ts | 13 ++++++++--- .../src/imageLoader/wadouri/loadImage.ts | 22 ++++++------------- .../src/BaseStreamingImageVolume.ts | 1 + packages/tools/src/utilities/index.ts | 2 ++ yarn.lock | 2 +- 6 files changed, 25 insertions(+), 19 deletions(-) diff --git a/common/reviews/api/tools.api.md b/common/reviews/api/tools.api.md index f40bfb7cd7..c4a5b28763 100644 --- a/common/reviews/api/tools.api.md +++ b/common/reviews/api/tools.api.md @@ -3478,6 +3478,9 @@ function pointInEllipse(ellipse: any, pointLPS: any, inverts?: Inverts): boolean // @public (undocumented) function pointInShapeCallback(imageData: vtkImageData | Types_2.CPUImageData, pointInShapeFn: ShapeFnCriteria, callback?: PointInShapeCallback, boundsIJK?: BoundsIJK_2): Array; +// @public (undocumented) +function pointInSurroundingSphereCallback(imageData: vtkImageData, circlePoints: [Types_2.Point3, Types_2.Point3], callback: PointInShapeCallback, viewport?: Types_2.IVolumeViewport): void; + // @public (undocumented) const pointsAreWithinCloseContourProximity: (p1: Types_2.Point2, p2: Types_2.Point2, closeContourProximity: number) => boolean; @@ -5361,6 +5364,7 @@ declare namespace utilities { getAnnotationNearPoint, getAnnotationNearPointOnEnabledElement, jumpToSlice, + pointInSurroundingSphereCallback, viewport, cine, clip_2 as clip, diff --git a/packages/core/src/requestPool/requestPoolManager.ts b/packages/core/src/requestPool/requestPoolManager.ts index 27f8393a3a..7cc83e5eb6 100644 --- a/packages/core/src/requestPool/requestPoolManager.ts +++ b/packages/core/src/requestPool/requestPoolManager.ts @@ -239,10 +239,17 @@ class RequestPoolManager { this.numRequests[type]++; this.awake = true; - requestDetails.requestFn()?.finally(() => { + const requestResult = requestDetails.requestFn(); + if (requestResult?.finally) { + requestResult.finally(() => { + this.numRequests[type]--; + this.startAgain(); + }); + } else { + // Handle non-async request functions too - typically just short circuit ones this.numRequests[type]--; - this.startAgain(); - }); + i--; + } } } diff --git a/packages/dicomImageLoader/src/imageLoader/wadouri/loadImage.ts b/packages/dicomImageLoader/src/imageLoader/wadouri/loadImage.ts index 47fdcc2b34..e628cdbb75 100644 --- a/packages/dicomImageLoader/src/imageLoader/wadouri/loadImage.ts +++ b/packages/dicomImageLoader/src/imageLoader/wadouri/loadImage.ts @@ -126,12 +126,7 @@ function loadImageFromDataSet( const pixelData = getPixelData(dataSet, frame); const transferSyntax = dataSet.string('x00020010'); - imagePromise = createImage( - imageId, - pixelData, - transferSyntax, - options - ); + imagePromise = createImage(imageId, pixelData, transferSyntax, options); } catch (error) { // Reject the error, and the dataSet reject({ @@ -177,13 +172,10 @@ function loadImage( const parsedImageId = parseImageId(imageId); options = Object.assign({}, options); - let loader = options.loader; - - if (loader === undefined) { - loader = getLoaderForScheme(parsedImageId.scheme); - } else { - delete options.loader; - } + // The options might have a loader, but it is a loader into the cache, so not + // the scheme loader, which is separate + delete options.loader; + const schemeLoader = getLoaderForScheme(parsedImageId.scheme); // if the dataset for this url is already loaded, use it, in case of multiframe // images, we need to extract the frame pixelData from the dataset although the @@ -194,7 +186,7 @@ function loadImage( */ const dataSet: DataSet = (dataSetCacheManager as any).get( parsedImageId.url, - loader, + schemeLoader, imageId ); @@ -210,7 +202,7 @@ function loadImage( // load the dataSet via the dataSetCacheManager const dataSetPromise = dataSetCacheManager.load( parsedImageId.url, - loader, + schemeLoader, imageId ); diff --git a/packages/streaming-image-volume-loader/src/BaseStreamingImageVolume.ts b/packages/streaming-image-volume-loader/src/BaseStreamingImageVolume.ts index ae52e6a262..38366232c0 100644 --- a/packages/streaming-image-volume-loader/src/BaseStreamingImageVolume.ts +++ b/packages/streaming-image-volume-loader/src/BaseStreamingImageVolume.ts @@ -453,6 +453,7 @@ export default class BaseStreamingImageVolume const { cachedFrames } = this; if (cachedFrames[imageIdIndex] === ImageQualityStatus.FULL_RESOLUTION) { + // The request framework handles non-promise returns, so just return here return; } diff --git a/packages/tools/src/utilities/index.ts b/packages/tools/src/utilities/index.ts index 95bf826b37..52ab0703e4 100644 --- a/packages/tools/src/utilities/index.ts +++ b/packages/tools/src/utilities/index.ts @@ -25,6 +25,7 @@ import { getSphereBoundsInfo } from './getSphereBoundsInfo'; import scroll from './scroll'; import { pointToString } from './pointToString'; import annotationFrameRange from './annotationFrameRange'; +import pointInSurroundingSphereCallback from './pointInSurroundingSphereCallback'; // name spaces import * as contours from './contours'; @@ -75,6 +76,7 @@ export { getAnnotationNearPoint, getAnnotationNearPointOnEnabledElement, jumpToSlice, + pointInSurroundingSphereCallback, viewport, cine, clip, diff --git a/yarn.lock b/yarn.lock index f73f1178ce..f581407e13 100644 --- a/yarn.lock +++ b/yarn.lock @@ -17399,7 +17399,7 @@ pkg-up@^3.1.0: version "1.1.0" resolved "https://codeload.github.com/ataft/plugin-image-zoom/tar.gz/8e1b866c79ed6d42cefc4c52f851f1dfd1d0c7de" dependencies: - medium-zoom "^1.0.4" + medium-zoom "^1.0.8" posix-character-classes@^0.1.0: version "0.1.1" From 6fe0e489747739a085520695887b5c1d20b12412 Mon Sep 17 00:00:00 2001 From: Bill Wallace Date: Tue, 9 Jan 2024 19:04:16 -0500 Subject: [PATCH 2/6] Update comments --- packages/core/src/requestPool/requestPoolManager.ts | 8 +++++++- .../dicomImageLoader/src/imageLoader/wadouri/loadImage.ts | 5 +++-- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/packages/core/src/requestPool/requestPoolManager.ts b/packages/core/src/requestPool/requestPoolManager.ts index 7cc83e5eb6..87616fce2f 100644 --- a/packages/core/src/requestPool/requestPoolManager.ts +++ b/packages/core/src/requestPool/requestPoolManager.ts @@ -239,7 +239,13 @@ class RequestPoolManager { this.numRequests[type]++; this.awake = true; - const requestResult = requestDetails.requestFn(); + let requestResult; + try { + requestResult = requestDetails.requestFn(); + } catch (e) { + // This is the only warning one will get, so need a warn message + console.warn('sendRequest failed', e); + } if (requestResult?.finally) { requestResult.finally(() => { this.numRequests[type]--; diff --git a/packages/dicomImageLoader/src/imageLoader/wadouri/loadImage.ts b/packages/dicomImageLoader/src/imageLoader/wadouri/loadImage.ts index e628cdbb75..7ccd598dae 100644 --- a/packages/dicomImageLoader/src/imageLoader/wadouri/loadImage.ts +++ b/packages/dicomImageLoader/src/imageLoader/wadouri/loadImage.ts @@ -172,9 +172,10 @@ function loadImage( const parsedImageId = parseImageId(imageId); options = Object.assign({}, options); - // The options might have a loader, but it is a loader into the cache, so not - // the scheme loader, which is separate + // The loader isn't transferable, so ensure it is deleted delete options.loader; + // The options might have a loader above, but it is a loader into the cache, + // so not the scheme loader, which is separate and defined by the scheme here const schemeLoader = getLoaderForScheme(parsedImageId.scheme); // if the dataset for this url is already loaded, use it, in case of multiframe From 1c15ec16a70a2a04015fcfdf1e84ad697ed44e52 Mon Sep 17 00:00:00 2001 From: Bill Wallace Date: Wed, 10 Jan 2024 08:12:37 -0500 Subject: [PATCH 3/6] Add docs to address a comment --- packages/core/src/requestPool/requestPoolManager.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/core/src/requestPool/requestPoolManager.ts b/packages/core/src/requestPool/requestPoolManager.ts index 87616fce2f..80f08c314f 100644 --- a/packages/core/src/requestPool/requestPoolManager.ts +++ b/packages/core/src/requestPool/requestPoolManager.ts @@ -254,6 +254,10 @@ class RequestPoolManager { } else { // Handle non-async request functions too - typically just short circuit ones this.numRequests[type]--; + // Decrement the index counter to add another request to the set of + // requests being sent on this sendRequests - usually an empty + // response here means that the request was cancelled so just add another + // request in it's place, without the "startAgain" handling. i--; } } From 585611a8202584f6c6ae5e19a373f322ba1e637e Mon Sep 17 00:00:00 2001 From: Bill Wallace Date: Wed, 10 Jan 2024 09:07:50 -0500 Subject: [PATCH 4/6] Updated the loop counter name so it is obvious what is happening. --- .../core/src/requestPool/requestPoolManager.ts | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/packages/core/src/requestPool/requestPoolManager.ts b/packages/core/src/requestPool/requestPoolManager.ts index 80f08c314f..ec3f7cf7b2 100644 --- a/packages/core/src/requestPool/requestPoolManager.ts +++ b/packages/core/src/requestPool/requestPoolManager.ts @@ -231,7 +231,11 @@ class RequestPoolManager { private sendRequests(type) { const requestsToSend = this.maxNumRequests[type] - this.numRequests[type]; - for (let i = 0; i < requestsToSend; i++) { + for ( + let requestsStarted = 0; + requestsStarted < requestsToSend; + requestsStarted++ + ) { const requestDetails = this.getNextRequest(type); if (requestDetails === null) { return false; @@ -254,11 +258,10 @@ class RequestPoolManager { } else { // Handle non-async request functions too - typically just short circuit ones this.numRequests[type]--; - // Decrement the index counter to add another request to the set of - // requests being sent on this sendRequests - usually an empty - // response here means that the request was cancelled so just add another - // request in it's place, without the "startAgain" handling. - i--; + // Decrement the index counter to redo this loop index as + // this request was cancelled or failed, so we still + // want to do the same number of requests. + requestsStarted--; } } } From 9a667a80338d6bcbb5c6203af83b1201a50ebc39 Mon Sep 17 00:00:00 2001 From: Bill Wallace Date: Wed, 10 Jan 2024 14:16:11 -0500 Subject: [PATCH 5/6] PR changes --- packages/core/src/requestPool/requestPoolManager.ts | 8 +++----- yarn.lock | 2 +- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/packages/core/src/requestPool/requestPoolManager.ts b/packages/core/src/requestPool/requestPoolManager.ts index ec3f7cf7b2..0146645835 100644 --- a/packages/core/src/requestPool/requestPoolManager.ts +++ b/packages/core/src/requestPool/requestPoolManager.ts @@ -230,10 +230,11 @@ class RequestPoolManager { private sendRequests(type) { const requestsToSend = this.maxNumRequests[type] - this.numRequests[type]; + let syncImageCount = 0; for ( let requestsStarted = 0; - requestsStarted < requestsToSend; + requestsStarted < requestsToSend + syncImageCount; requestsStarted++ ) { const requestDetails = this.getNextRequest(type); @@ -258,10 +259,7 @@ class RequestPoolManager { } else { // Handle non-async request functions too - typically just short circuit ones this.numRequests[type]--; - // Decrement the index counter to redo this loop index as - // this request was cancelled or failed, so we still - // want to do the same number of requests. - requestsStarted--; + syncImageCount++; } } } diff --git a/yarn.lock b/yarn.lock index f581407e13..3d3e3fd8ba 100644 --- a/yarn.lock +++ b/yarn.lock @@ -15160,7 +15160,7 @@ media-typer@0.3.0: resolved "https://registry.yarnpkg.com/media-typer/-/media-typer-0.3.0.tgz#8710d7af0aa626f8fffa1ce00168545263255748" integrity sha512-dq+qelQ9akHpcOl/gUVRTxVIOkAJ1wR3QAvb4RsVjS8oVoFjDGTc679wJYmUmknUF5HwMLOgb5O+a3KxfWapPQ== -medium-zoom@^1.0.4: +medium-zoom@^1.0.8: version "1.1.0" resolved "https://registry.yarnpkg.com/medium-zoom/-/medium-zoom-1.1.0.tgz#6efb6bbda861a02064ee71a2617a8dc4381ecc71" integrity sha512-ewyDsp7k4InCUp3jRmwHBRFGyjBimKps/AJLjRSox+2q/2H4p/PNpQf+pwONWlJiOudkBXtbdmVbFjqyybfTmQ== From 9cec94b05a2d4d13929e73eb969adee1db46acf7 Mon Sep 17 00:00:00 2001 From: Bill Wallace Date: Thu, 11 Jan 2024 09:08:10 -0500 Subject: [PATCH 6/6] PR review comments --- packages/core/src/requestPool/requestPoolManager.ts | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/packages/core/src/requestPool/requestPoolManager.ts b/packages/core/src/requestPool/requestPoolManager.ts index 0146645835..7df1283918 100644 --- a/packages/core/src/requestPool/requestPoolManager.ts +++ b/packages/core/src/requestPool/requestPoolManager.ts @@ -232,11 +232,7 @@ class RequestPoolManager { const requestsToSend = this.maxNumRequests[type] - this.numRequests[type]; let syncImageCount = 0; - for ( - let requestsStarted = 0; - requestsStarted < requestsToSend + syncImageCount; - requestsStarted++ - ) { + for (let i = 0; i < requestsToSend; i++) { const requestDetails = this.getNextRequest(type); if (requestDetails === null) { return false; @@ -263,6 +259,9 @@ class RequestPoolManager { } } } + if (syncImageCount) { + this.startAgain(); + } return true; }