serhiy-bzhezytskyy commented on PR #4655: URL: https://github.com/apache/solr/pull/4655#issuecomment-5131364215
@dsmiley the deferral does more than tidy the code — it removes the leak by construction for one whole class of failure. I measured it with a counting executor on a refused connection: before `e534b380e` one body task is submitted and then has to be released; after it, **zero** (`expected:<0> but was:<1>` against the previous commit). So the original SOLR-17707 guard now only covers "body requested, then the connection dropped", which is the narrower case it should have been. Two things I checked while I was in here, both fine: - the mechanism sentence still holds. I mutated the fix on `main` to keep only `cancel(true)` and the SOLR-17707 test fails with `content-writing thread leaked, still blocked after failure` — so `close(sink)` is still load-bearing, and moving the comment along with the code was right. - `contentWritingSink` is assigned before `contentWritingFuture`, so a `releaseContentWriting()` arriving between them finds a null future and an already-closed sink. Over 200 runs that is 200 writers exiting on `IOException` with the pool fully drained — log noise, not a leak. Not worth a change. The one thing missing is a test for the new property. I have the one above if you want it, against this branch or as a follow-up. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
