stillalex commented on code in PR #1780:
URL: https://github.com/apache/solr/pull/1780#discussion_r1265951622
##########
solr/core/src/java/org/apache/solr/servlet/ServletUtils.java:
##########
@@ -216,16 +217,19 @@ static void rateLimitRequest(
}
if (!accepted) {
- String errorMessage =
- "Too many requests for this request type."
- + "Please try after some time or increase the quota for this
request type";
-
- response.sendError(429, errorMessage);
+ response.sendError(ErrorCode.TOO_MANY_REQUESTS.code,
RateLimitManager.ERROR_MESSAGE);
+ return;
}
+ needsCleanup = false;
// todo: this shouldn't be required, tracing and rate limiting should be
independently
// composable
traceHttpRequestExecution2(request, response, limitedExecution, trace);
} finally {
+ if (needsCleanup) {
+ consumeInputFully(request, response);
+ SolrRequestInfo.reset();
+ SolrRequestParsers.cleanupMultipartFiles(request);
Review Comment:
thank you for taking a look. I agree with the control flow idea 100%. it is
very difficult to work around code that is so poorly set in place.
I introduced this flag because ` traceHttpRequestExecution2` does resource
cleanup "sometimes", take a look (I actually think the 'else' block in there is
dead code, but it's not relevant to this PR).
ideally this entire block "rate limiter method calling into trace method
calling into mystery method" would be wrapped into a try/finally with correct
cleanup but it's not so I added this flag which was "best effort cleanup" in
case the trace method does not get called.
Also, is there an actual test that verifies correct resource cleanup? I am
happy to extend it with this change.
If you have stronger opinions (proposing a proper refactoring) and are good
for a review, I am happy to dive deeper, but so far there are not too many
reviewers available.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
