ISPN-6356 Wrap removal of default HR liteners into security actions #4165
Conversation
|
|
||
| @Override | ||
| public Void run() { | ||
| listenable.removeListener(listener); |
There was a problem hiding this comment.
Perhaps we could add some not null checks here? I think it would be the best to ignore this instruction when listenable is null.
There was a problem hiding this comment.
instead of doing nothing when listenable is null, we should at least log it, as there's probably something wrong, null is definitely expected here. Looking into other security actions, non of them contains logging or other checks - I guess it's because of performance, as these actions can be used quite frequently - is this correct @tristantarrant ?
If I'm wrong and there are no performance concerns, no problem to add it.
|
I think it would be good to have some test for it... What do you think? |
test for what actually? The functionality is basically tested in many HR server tests (these listeners are added automatically) and this PR just removes warning from log - not sure how to test it. |
|
I took another look at the stacktrace in the JIRA and I think I now understand what's going on... integrating into both - master and 8.2.x |
|
Ok, it has been integrated previously... Thanks Vojtech! |
https://issues.jboss.org/browse/ISPN-6356