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

Add configurable Error handling for HTTP requests #1598

Closed
swederik opened this issue Apr 3, 2020 · 10 comments
Closed

Add configurable Error handling for HTTP requests #1598

swederik opened this issue Apr 3, 2020 · 10 comments
Assignees
Labels
IDC:priority Items that the Imaging Data Commons wants to help sponsor
Projects

Comments

@swederik
Copy link
Member

swederik commented Apr 3, 2020

For the NCI Imaging Data Commons project, users will be able to access data without logging in, but a data usage limit will be put in place to prevent users from abusing the system to directly download studies.

Once the user reaches the data usage limit, the HTTP requests to the proxy will begin returning HTTP 429 Too Many Requests. The exact behaviour we want once this has been reached is currently unknown, but it will likely be something like showing a modal or redirecting the user to the IDC homepage or a form to request more resources.

In general, though, we should be able to configure error handling at the HTTP level for all of the requests that OHIF is making to the PACS. Although this is not in scope in this ticket, for other resources we can resurrect #1434 later on.

This means we will likely need to add error handling in calls to Cornerstone's loadAndCacheImage, and possibly at the level of cornerstoneWADOImageLoader and dicomweb-client.

I can imagine this would be useful for other cases, e.g. how to handle users suddenly receiving 401 responses from PACS.

@swederik swederik added the IDC:candidate Possible feature requests for discussion, and if we agree, they can be relabeled IDC:priority label Apr 3, 2020
@swederik swederik added this to Priority in IDC via automation Apr 3, 2020
@swederik swederik moved this from Priority to Candidate in IDC Apr 3, 2020
@salimkanoun
Copy link
Contributor

Hi,
I think the problem I just reported is linked to this one :
#1606

Probably there is a way to hit two birds with one stone,

Best regards,

Salim

@JamesAPetts
Copy link
Member

This might be useful, error handlers can be added to cornerstone events fired by loadAndCacheImage here in cornerstoneTools.

https://github.com/cornerstonejs/cornerstoneTools/blob/master/src/stateManagement/loadHandlerManager.js

@wlongabaugh
Copy link

@swederik Per your question on IDC Slack, it looks good to go as an IDC:Priority, but I don't believe I have permissions here to modify labels or status. Thanks.

@fedorov fedorov added IDC:priority Items that the Imaging Data Commons wants to help sponsor and removed IDC:candidate Possible feature requests for discussion, and if we agree, they can be relabeled IDC:priority labels Apr 7, 2020
@swederik swederik moved this from Candidate to Priority in IDC Apr 7, 2020
@fedorov fedorov moved this from Priority to In progress in IDC Apr 9, 2020
@JamesAPetts JamesAPetts moved this from In progress to Priority in IDC Apr 9, 2020
@fedorov fedorov moved this from Priority to In progress in IDC Apr 9, 2020
@fedorov fedorov moved this from In progress to Priority in IDC Apr 9, 2020
@JamesAPetts JamesAPetts moved this from Priority to In progress in IDC Apr 9, 2020
@JamesAPetts JamesAPetts assigned JamesAPetts and unassigned dannyrb Apr 16, 2020
@JamesAPetts JamesAPetts moved this from In progress to Done in IDC Apr 23, 2020
@wlongabaugh
Copy link

@JamesAPetts I don't seem to have a way to distinguish between the 429 status returned by the server and other HTTP errors; the error.status value that shows up in the handler is always equal to 0, presumably what is returned by XMLHttpRequest. I have triple-checked the server response and I am pretty sure it is sending back 429. I have only recently been able to go back and work on this. Note the idc-sandbox-002 proxy is set to trigger a 429 after about only 16MB of daily downloads.

@fedorov fedorov reopened this Jun 24, 2020
IDC automation moved this from Done to In progress Jun 24, 2020
@JamesAPetts
Copy link
Member

Hey @wlongabaugh, Looking here under dicomweb-client, this response code really is what is being given by the XMLHttpRequest: https://github.com/dcmjs-org/dicomweb-client/blob/ee20303c5dc8d40955685e0e5b0021bb8ead27e2/src/api.js#L142.

XMLHttpRequest is native web api, and about as much control as we have on the client.

A zero error code can come from:

  • Before the request completes, the value of status is 0.
  • Browsers also report a status of 0 in case of XMLHttpRequest errors.

As this section is wrapped in a request.readyState === 4 if block , it cannot be the first option, as state 4 is ready, this must mean its hitting an XMLHttpRequest error.

I can confirm the 429 is being sent if I hit the requested series data directly in the browser
I just had a check and locally I am hitting a cors error when the 429 is being sent back, which the browser will always cast to a status code of 0. We don't get cors errors when our quota is not exceeded however.

https://idc-testing.appspot.com/projects/chc-tcia/locations/us-central1/datasets/tcga-stad/dicomStores/tcga-stad/study/1.3.6.1.4.1.14519.5.2.1.6354.4025.201007175423148952381133335609

When you return the 429 are you attaching the same Access-Control-Allow-Headers as you do when a 200 is returned?

Kind regards,
James

@fedorov
Copy link
Member

fedorov commented Jun 24, 2020

Thanks for the quick response @JamesAPetts, I appreciate it!

@wlongabaugh
Copy link

Thanks for the analysis; it is very helpful. I don't think I am attaching the headers, and will go do that.

@wlongabaugh
Copy link

I have added the CORS headers to the 429 response, and a check indicates that it is getting through now. The error handler for the viewer in idc-dev is now set up to report quota exceeded only when a 429 arrives. Still need to test it with a low daily limit set up to confirm the issue can be closed. Thanks.

@fedorov
Copy link
Member

fedorov commented Jul 3, 2020

@wlongabaugh are we all set with this issue - can it be closed?

@wlongabaugh
Copy link

I wanted to check that the page actually triggers when the quota is hit, and was waiting to test that, but wasn't getting the chance to put a low quota in dev. But I guess you got the (crappy original) quota page yesterday already (correct?) with the actual 4GB limit in place, so yeah it does work as designed. You can close.

@swederik swederik closed this as completed Jul 6, 2020
IDC automation moved this from In progress to Done Jul 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
IDC:priority Items that the Imaging Data Commons wants to help sponsor
Projects
IDC
  
Done
Development

No branches or pull requests

6 participants