-
Notifications
You must be signed in to change notification settings - Fork 317
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
Changes from 1 commit
a0caa98
6fe0e48
1c15ec1
585611a
9a667a8
9cec94b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -239,10 +239,17 @@ class RequestPoolManager { | |
this.numRequests[type]++; | ||
this.awake = true; | ||
|
||
requestDetails.requestFn()?.finally(() => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a leak on the numRequests pool size. |
||
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--; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is odd, you are decrementing the loop iterator inside the loop. Are you sure about this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
} | ||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
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.