psteitz edited a comment on pull request #67: URL: https://github.com/apache/commons-pool/pull/67#issuecomment-803663669
Sorry for taking so long to review this. Looks good to me. Thanks and nice work. I only have one comment. The iteration in the top loop in removeAbandoned is over poolMap.entrySet. This set may be modified during execution of the loop (due to adding / dropping keyed pools). I am not sure the behavior will be consistent / predictable in all cases. Could be I am wrong on this in which case this comment can be ignored. It might be better to take some kind of snapshot at the top of the loop. I don't think any additional sync is needed (beyond what you have using the victims' monitors), and if the loop behaves predictably (skipping removals, possibly missing adds), the current code is OK. Missing adds is not likely a problem since instances in new pools are not likely to have timed out and skipping removals similarly is no problem, since pools don't get dropped unless they are empty. I am just worried about strange things happening due to behavior being undefined. -- 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. For queries about this service, please contact Infrastructure at: [email protected]
