You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@roller.apache.org by GitBox <gi...@apache.org> on 2021/02/11 00:17:13 UTC
[GitHub] [roller] nuzayats opened a new pull request #75: Ipbanlist fixes
nuzayats opened a new pull request #75:
URL: https://github.com/apache/roller/pull/75
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
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [roller] nuzayats commented on pull request #75: Ipbanlist fixes
Posted by GitBox <gi...@apache.org>.
nuzayats commented on pull request #75:
URL: https://github.com/apache/roller/pull/75#issuecomment-846311708
> 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 which concerns you about 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:
users@infra.apache.org
[GitHub] [roller] mbien commented on pull request #75: Ipbanlist fixes
Posted by GitBox <gi...@apache.org>.
mbien commented on pull request #75:
URL: https://github.com/apache/roller/pull/75#issuecomment-802596322
@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).
--
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:
users@infra.apache.org
[GitHub] [roller] snoopdave commented on pull request #75: Ipbanlist fixes
Posted by GitBox <gi...@apache.org>.
snoopdave commented on pull request #75:
URL: https://github.com/apache/roller/pull/75#issuecomment-779481465
> 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.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [roller] mbien commented on pull request #75: Ipbanlist fixes
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [roller] nuzayats commented on pull request #75: Ipbanlist fixes
Posted by GitBox <gi...@apache.org>.
nuzayats commented on pull request #75:
URL: https://github.com/apache/roller/pull/75#issuecomment-778521097
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
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [roller] nuzayats commented on pull request #75: Ipbanlist fixes
Posted by GitBox <gi...@apache.org>.
nuzayats commented on pull request #75:
URL: https://github.com/apache/roller/pull/75#issuecomment-846327275
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 :(
--
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:
users@infra.apache.org
[GitHub] [roller] snoopdave commented on pull request #75: Ipbanlist fixes
Posted by GitBox <gi...@apache.org>.
snoopdave commented on pull request #75:
URL: https://github.com/apache/roller/pull/75#issuecomment-803639999
LGTM
--
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:
users@infra.apache.org
[GitHub] [roller] mbien commented on pull request #75: Ipbanlist fixes
Posted by GitBox <gi...@apache.org>.
mbien commented on pull request #75:
URL: https://github.com/apache/roller/pull/75#issuecomment-846333744
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 :)
--
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:
users@infra.apache.org
[GitHub] [roller] nuzayats edited a comment on pull request #75: Ipbanlist fixes
Posted by GitBox <gi...@apache.org>.
nuzayats edited a comment on pull request #75:
URL: https://github.com/apache/roller/pull/75#issuecomment-846311708
> 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?
--
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:
users@infra.apache.org
[GitHub] [roller] mbien commented on pull request #75: Ipbanlist fixes
Posted by GitBox <gi...@apache.org>.
mbien commented on pull request #75:
URL: https://github.com/apache/roller/pull/75#issuecomment-777361323
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.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [roller] mbien merged pull request #75: Ipbanlist fixes
Posted by GitBox <gi...@apache.org>.
mbien merged pull request #75:
URL: https://github.com/apache/roller/pull/75
--
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:
users@infra.apache.org
[GitHub] [roller] mbien commented on pull request #75: Ipbanlist fixes
Posted by GitBox <gi...@apache.org>.
mbien commented on pull request #75:
URL: https://github.com/apache/roller/pull/75#issuecomment-846322119
> > 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)
--
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:
users@infra.apache.org
[GitHub] [roller] nuzayats commented on pull request #75: Ipbanlist fixes
Posted by GitBox <gi...@apache.org>.
nuzayats commented on pull request #75:
URL: https://github.com/apache/roller/pull/75#issuecomment-782568242
> 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.
----------------------------------------------------------------
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:
users@infra.apache.org