fix: Lockup in request pool when returning non promise result

[wayfarer3130]

Context

The wrong loader was being used in the URI loadImage call, and there was a bug in the request pool manager that failed to start requests when the request returned an empty result instead of a promise (eg short circuit)

Changes & Results

Fix the request pool manager to ensure the counts are decremented even on no response
Fix the loadImage to use the original scheme loader (the options loader was never specified, so it wasn't relevant, and was slightly different in the newer version, so wasn't working)

Testing

Checklist

PR

• [] My Pull Request title is descriptive, accurate and follows the
  semantic-release format and guidelines.

Code

• [] My code has been well-documented (function documentation, inline comments,
  etc.)

• [] I have run the yarn build:update-api to update the API documentation, and have
  committed the changes to this PR. (Read more here https://www.cornerstonejs.org/docs/contribute/update-api)

Public Documentation Updates

• [] The documentation page has been updated as necessary for any public API
  additions or removals.

Tested Environment

• [] "OS:
• [] "Node version:
• [] "Browser:

[netlify]

✅ Deploy Preview for cornerstone-3d-docs ready!

Name	Link
🔨 Latest commit	9cec94b
🔍 Latest deploy log	https://app.netlify.com/sites/cornerstone-3d-docs/deploys/659ff677e8314600087f4acc
😎 Deploy Preview	https://deploy-preview-990--cornerstone-3d-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[wayfarer3130]

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

[wayfarer3130]

There was a leak on the numRequests pool size.

[sedghi]

This is odd, you are decrementing the loop iterator inside the loop. Are you sure about this?

[wayfarer3130]

Yes, because I don't want to call startAgain, which would run the entire loop again, but I want to add anything the given number of requests to the request pool. The send request really on throws or returns undefined when it is an empty request, so for those the request can simply be skipped and we "redo" that index - except as another request.

[sedghi]

is this part of this fix or is unrelated and potential bug in future, since I don't think the approach is fine. if it is not part of this fix let's worry about that later

[sedghi]

Thanks a lot!

[sgielen]

@sedghi @wayfarer3130 this PR removes the wadouri.loadImage() loader option, which was the only way I knew how to load a Cornerstone image from an ArrayBuffer we had already loaded from another source (see also the code in #886, #888, #889). Now, the loader option is ignored. What is now the preferred way to load an image from an ArrayBuffer?

[sedghi]

@sgielen So how do we test it?

[sgielen]

@sgielen So how do we test it?

I think we should look at the design first and see where the root issue stems from.

By design, cornerstone3d ImageLoaders receive an ID, start some download, import those bytes into an ImageObject, and return it. The parsing of those download bytes (e.g. DICOM, NIfTI format) is considered to be the ImageLoader responsibility as well, since every format is different. However, Cornerstone still needs generic metadata from the image, so ImageLoaders also register MetadataProviders which can provide the responses to generic questions of many types (width/height, pixel/voxel sizes, LUTs / window settings etc). So, the ImageLoader not only loads pixel data, but also converts metadata into a format recognized by the rest of Cornerstone.

So the two (current) purposes of an ImageLoader are to return image pixels, and to keep its metadata in its cache for as long as the image can be handled.

Now, my particular issue is that this smartness is included in the Cornerstone DICOM image loader, but in a way that makes it difficult to use in another ImageLoader. The metadata cache is included in the WADOURI implementation runtime as a global variable, and the metadata logic code is tightly coupled with this cache variable. This means either my DICOM datasets need to be in its cache, or I need to copy all of the DICOM -> Cornerstone metadata logic into my own project to make it use my own cache. I can't just call any of the parsing functions without depending on the WADOURI cache as well, so my ImageLoader is either too tightly coupled or does not reuse code properly.

So far, I chose to use the WADOURI cache even if I don't use the WADOURI loader, so I don't need to replicate all the code. This is why, in my image loader, I wrap the WADOURI loader, overwriting its loader option. Now that it's gone, I probably can work around this by using other parts of the DICOM image loader, such as calling dataSetCacheManager.load() myself, but it does still feel a bit wonky and not using the loader as intended. Perhaps this is a good time to look at it from some distance and see what we could improve.

I believe that ideally, all of the DICOM functions in the dicomImageLoader would be separate, testable chunks, to be used by WADOURI, perhaps WADORS and other image loaders that eventually work with DICOM bytes.

If you agree, perhaps I can file an issue for it and I can propose a refactoring strategy.

[sedghi]

@sgielen Sorry I meant do you have steps or better code that you can share to reproduce

[sgielen]

@sgielen Sorry I meant do you have steps or better code that you can share to reproduce

Oh, yes, certainly! You can find an example e.g. in #888 (https://sjorsgielen.nl/cornerstone3d/webworkermanager-init.zip). The code in the .zip file downloads DICOMs using fetch, as an example of an alternative non-WADOURI method to get DICOMs, then tries to load them using wadouri. It's intended to be a testcase for another issue, but if you set initWebWorkers=false at the top of the file then it works well in Cornerstone3d 1.21.2. I haven't tested it but it should also work well with Cornerstone3d 1.44.2, the last version before this PR. Since the loader option to WADOURI is gone after this PR, it is broken on current latest release / main branch: it will perform a WADOURI request instead of using fetch, and that request will of course fail.

So, if the loader option is brought back it should work well again on the newest version, but if the loader option should not be brought back, we could also use this as a starting point to design a better way to load DICOMs in a third-party ImageLoader.

For brevity, this is the relevant code in the .zip file:

// Since we use a custom Image Loader, use cornerstoneDICOMImageLoader to
// turn this custom method into Cornerstone images
cornerstoneDICOMImageLoader.wadouri.dataSetCacheManager.purge();
cornerstone.registerImageLoader(
  'custom',
  (imageId: string) => {
     const filename = imageId.substring('custom:'.length);
     return cornerstoneDICOMImageLoader.wadouri.loadImage(imageId, {
       loader: () => {
         console.log(filename);
         return fetch("/" + filename).then((r) => r.arrayBuffer());
       },
       preScale: {
         enabled: true,
       },
     });
  },
);

[sgielen]

@sedghi any thoughts as to bringing back wadouri.loadImage or implementing an alternative? :)

[jlopes90]

@sedghi

I'm using options.loader and now it's removed. Can I create a pull request to return options.loader?

Because I'm using options.loader, for example:
It's very similar to wadouri.loadFileRequest, but I created the store manager is like "fileManager" so I can see the private files inside it. It's not just dicomfile, also videofile, imagefile, niftifile and etc.

/**
 * Load Image From Dicom File
 * @param {string} imageId
 * @returns {object}
 */
function loadImageFromDicomFile(imageId) {

    // Function Loader
    const fnLoader = function (uri, imageId) {
        const index = imageId.replace('dicomfile:', '');

        // Get file
        const file = exStore.manager.get(index); // it's like fileManager

        // Promise
        const promise = new Promise(function (resolve, reject) {

            //
            const fileReader = new FileReader();

            //
            fileReader.onload = function (evt) {
                resolve(evt.target.result);
            };

            //
            fileReader.onerror = function (err) {
                reject(err);
            };

            //
            fileReader.readAsArrayBuffer(file);
        });

        // Return
        return promise;
    };

    //
    return wadouri.loadImage(imageId, {
        loader: fnLoader
    });
}

[jlopes90]

My opinion in the future, fileManager should move to package/core because it's not just dicomfile, there will also be imagefile, niftifile, videofile, etc.

[sgielen]

I have two PRs open where this topic is being progressed:

• docs(image loader/metadata provider): Clarify concept + how-to documentation. #1508
• feat(image loaders): Allow reusing DICOM image loader code for custom image loaders #1509

Edit: These PRs have been merged. On cornerstone3D 2.0, this issue is resolved as there is now more complete documentation and an example of exactly this use-case.