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: Lockup in request pool when returning non promise result #990

Merged
merged 6 commits into from
Jan 11, 2024
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
4 changes: 4 additions & 0 deletions common/reviews/api/tools.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -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<PointInShape>;

// @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;

Expand Down Expand Up @@ -5361,6 +5364,7 @@ declare namespace utilities {
getAnnotationNearPoint,
getAnnotationNearPointOnEnabledElement,
jumpToSlice,
pointInSurroundingSphereCallback,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This got removed somehow and I'm just adding it back as OHIF still uses it.

viewport,
cine,
clip_2 as clip,
Expand Down
23 changes: 20 additions & 3 deletions packages/core/src/requestPool/requestPoolManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,7 @@ class RequestPoolManager {

private sendRequests(type) {
const requestsToSend = this.maxNumRequests[type] - this.numRequests[type];
let syncImageCount = 0;

for (let i = 0; i < requestsToSend; i++) {
const requestDetails = this.getNextRequest(type);
Expand All @@ -239,12 +240,28 @@ class RequestPoolManager {
this.numRequests[type]++;
this.awake = true;

requestDetails.requestFn()?.finally(() => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There was a leak on the numRequests pool size.

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]--;
this.startAgain();
});
} else {
// Handle non-async request functions too - typically just short circuit ones
this.numRequests[type]--;
this.startAgain();
});
syncImageCount++;
}
}
}
if (syncImageCount) {
this.startAgain();
}

return true;
}
Expand Down
23 changes: 8 additions & 15 deletions packages/dicomImageLoader/src/imageLoader/wadouri/loadImage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand Down Expand Up @@ -177,13 +172,11 @@ 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 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
// images, we need to extract the frame pixelData from the dataset although the
Expand All @@ -194,7 +187,7 @@ function loadImage(
*/
const dataSet: DataSet = (dataSetCacheManager as any).get(
parsedImageId.url,
loader,
schemeLoader,
imageId
);

Expand All @@ -210,7 +203,7 @@ function loadImage(
// load the dataSet via the dataSetCacheManager
const dataSetPromise = dataSetCacheManager.load(
parsedImageId.url,
loader,
schemeLoader,
imageId
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
2 changes: 2 additions & 0 deletions packages/tools/src/utilities/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -75,6 +76,7 @@ export {
getAnnotationNearPoint,
getAnnotationNearPointOnEnabledElement,
jumpToSlice,
pointInSurroundingSphereCallback,
viewport,
cine,
clip,
Expand Down
4 changes: 2 additions & 2 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -15160,7 +15160,7 @@ [email protected]:
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==
Expand Down Expand Up @@ -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"
Expand Down