Ipbanlist fixes

[nuzayats]

Fixes some concurrency issues in the IPBanList class:

• It calls HashSet#contains() from outside a synchronized block: it might not be able to catch a change made by a different thread
• The bannedIps field is not volatile even though multiple threads can set it to a new instance from loadBannedIps(): other threads might not see the updated instance

There are also some minor code improvements suggested by IntelliJ

[mbien]

looks good, some remarks:

• volatile could be removed again if the Set is made final (and cleared + reused in loadBannedIps())
• loadBannedIpsIfNeeded is only called with forceLoad set to false -> opportunity to be simplified.

sidenote:
I noticed that the IPBanList is indirectly polling the modification time of the file via isBanned(). Maybe it could be polled only once per second? I don't know the exact requirements for this feature however. Maybe @snoopdave could help.

[nuzayats]

Hello Michael, thanks for the review :)

volatile could be removed again if the Set is made final (and cleared + reused in loadBannedIps())

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

loadBannedIpsIfNeeded is only called with forceLoad set to false -> opportunity to be simplified.

Agreed. Removed at a5417a5

[mbien]

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

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.

[snoopdave]

looks good, some remarks:

• volatile could be removed again if the Set is made final (and cleared + reused in loadBannedIps())
• loadBannedIpsIfNeeded is only called with forceLoad set to false -> opportunity to be simplified.

sidenote:
I noticed that the IPBanList is indirectly polling the modification time of the file via isBanned(). Maybe it could be polled only once per second? I don't know the exact requirements for this feature however. Maybe @snoopdave could help.

Even once per second seems a bit excessive.

[nuzayats]

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.

Removed Mockito from pom.xml at 15bf473 , I thought I would need it but it turned out I didn't for now.

As for AssertJ, I'd like to keep it as it's much better than standard ones.

[mbien]

@nuzayats thanks!

I do like fluent interfaces too, just that i don't feel that it adds enough benefits in this particular case, to justify a new test dependency (just for one small junit test).

[snoopdave]

LGTM

[nuzayats]

i don't feel that it adds enough benefits in this particular case, to justify a new test dependency (just for one small junit test).

For this one at least it adds some benefit I think for example:

assertThat(readIpBanList()).containsOnly("10.0.0.1");

The standard assertion APIs don't provide such a collection verification if I'm not mistaken. Also to me having a new test dependency is not such a big deal but is there something concerning about it?

[mbien]

i don't feel that it adds enough benefits in this particular case, to justify a new test dependency (just for one small junit test).

For this one at least it adds some benefit I think for example:

assertThat(readIpBanList()).containsOnly("10.0.0.1");

The standard assertion APIs don't provide such a collection verification if I'm not mistaken.

assertTrue(list.contains("10.0.0.1"));
assertTrue(list.size() == 1);

or if you prefer one line

assertEquals(List.of("10.0.0.1"), list);

Also to me having a new test dependency is not such a big deal but is there something concerning about it?

Its far less problematic than a runtime dependency - i agree. Still, it has to pull its weight. Old projects tend to accumulate dependencies and removing them is always a little bit more difficult than adding new ones. More dependencies mean more maintenance time spent to keep them updated and detect abandoned projects to migrate away from.

The red flags here are that this library is only used by a single (small) test, and it is also not convincing to me that it is really necessary for that test.

However the impact is little (and it has no transitive dependencies), so if you really want it, I am ok to merge your changes. (thanks again for the contribution btw, esp also for adding a test case)

[nuzayats]

Removed AssertJ at 9dca2da .

But it sounds like I will have to stick with the basic or outdated assertion APIs in Roller development unless there is a chance to write substantial amount of new code and tests even though there is a much better assertion library available already today. The points you raised are valid but writing unit tests with fundamental assertTrue() and assertEquals() for collection verifications in 2021 is kind of sad :(

[mbien]

thanks for understanding. I am not opposed to AssertJ, if we have extensive collection based tests in future we can still add it so we get some proper use out of it. But maybe not for two lines delta :)