mbien commented on pull request #75:
URL: https://github.com/apache/roller/pull/75#issuecomment-778848074
> In order to reuse the Set that way, I think we need to clear and populate
it somehow atomically, which I'm not sure how, because otherwise it will
introduce a time window where isBanned() gets called while the Set is empty
which sounds like some kind of vulnerability. Based on that, I guess using
volatile this way is kind of reasonable
you are right. This would require a temporary map and iterating through the
concurrent map which causes also issues.
My initial thought was to keep volatile but make the map immutable (swap on
write), since its rarely updated but read in every request. But I don't know
how the file is used in practice - so lets keep your solution.
> > loadBannedIpsIfNeeded is only called with forceLoad set to false ->
opportunity to be simplified.
>
> Agreed. Removed at
[a5417a5](https://github.com/apache/roller/commit/a5417a5c3a6b7df83931871cc9b3721da19a97fd)
awesome
another thing:
are the new test dependencies needed? The test seems simple enough to do
fine with the standard junit asserts. I would try to avoid adding new
dependencies unless its worth it.
----------------------------------------------------------------
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:
us...@infra.apache.org
