New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Address severe issues with AlarmCallbacks #1224

Closed
wants to merge 4 commits into
base: 1.1
from

Conversation

Projects
None yet
2 participants
@lennartkoopmann
Member

lennartkoopmann commented Jun 5, 2015

This PR fixes #1221 and #1222. Please review and merge.

I thoroughly tested with all alert condition types triggering the Slack alarm callback plugin and the PagerDuty alarm callback plugin.

(this is a replacement for #1223 which was accidentally issued against master)

@joschi joschi added this to the 1.1.2 milestone Jun 7, 2015

@Override
public List<Message> getSearchHits() {
return null;

This comment has been minimized.

@joschi

joschi Jun 7, 2015

Contributor

This should return Collections#emptyList() instead of null to prevent accidental NullPointerExceptions in the caller.

This comment has been minimized.

@lennartkoopmann

lennartkoopmann Jun 8, 2015

Member

Indeed. Thanks!

@Override
public List<Message> getSearchHits() {
return searchHits;

This comment has been minimized.

@joschi

joschi Jun 7, 2015

Contributor

I'm not sure if this class is ever used concurrently from multiple threads, but if it was, this should either return a copy of the list (ImmutableList#copyOf()) or change the searchHits field to a CopyOnWriteArrayList.

This comment has been minimized.

@lennartkoopmann

lennartkoopmann Jun 8, 2015

Member

Agreed! We should make it a copy just to guarantee that other callbacks receive a list that has not been modified.

@Override
public List<Message> getSearchHits() {
return searchHits;

This comment has been minimized.

@joschi

joschi Jun 7, 2015

Contributor

I'm not sure if this class is ever used concurrently from multiple threads, but if it was, this should either return a copy of the list (ImmutableList#copyOf()) or change the searchHits field to a CopyOnWriteArrayList.

This comment has been minimized.

@lennartkoopmann

lennartkoopmann Jun 8, 2015

Member

See above.

@Override
public List<Message> getSearchHits() {
return this.searchHits;

This comment has been minimized.

@joschi

joschi Jun 7, 2015

Contributor

See above…

This comment has been minimized.

@lennartkoopmann

lennartkoopmann Jun 8, 2015

Member

See above.

This comment has been minimized.

@lennartkoopmann
@@ -19,6 +19,7 @@
import com.fasterxml.jackson.annotation.JsonIgnore;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Lists;
import org.graylog2.plugin.Message;

This comment has been minimized.

@joschi

joschi Jun 7, 2015

Contributor

Unused import.

This comment has been minimized.

@lennartkoopmann

@joschi joschi self-assigned this Jun 7, 2015

@lennartkoopmann

This comment has been minimized.

Member

lennartkoopmann commented Jun 8, 2015

Thanks Jochen! :) Addressed your remarks in 17110a9.

@lennartkoopmann

This comment has been minimized.

Member

lennartkoopmann commented Jun 8, 2015

I made getSearchHits() return an empty ArrayList instead of an empty ImmutableList because that was the behavior before the changes and I cannot guarantee that no plugin out there is not trying to directly work on that list.

@joschi

This comment has been minimized.

@joschi

This comment has been minimized.

@joschi

This comment has been minimized.

Using Collections#emptyList() will get rid of the unchecked cast warning.

@joschi joschi closed this in 9a01820 Jun 8, 2015

@joschi joschi deleted the fix-issues-1221-1222 branch Jun 8, 2015

@joschi joschi added bug and removed ready-for-review labels Jun 8, 2015

@lennartkoopmann

This comment has been minimized.

Member

lennartkoopmann commented Jun 9, 2015

Thank you! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment