Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Suggestion: default to not sending event if there is an error in the modules callback #10671

Closed
benparsons opened this issue Aug 23, 2021 · 7 comments

Comments

@benparsons
Copy link
Member

A developer working on a new project using the modules system reports:

if the module fails for some reason (e.g. with a runtime error in the Python code), the event is let through. This is worrying as it means that we don't have control on what the module can block. Is there a way to turn this around and have the module block the event by default in case of error?

The problem here is clearly that outside of the module interface it's not known which event types needed to be handled by the module. Is there a way we can flag events which have not been handled by the module callback and fail them?

@babolivier

This comment has been minimized.

@babolivier
Copy link
Contributor

The problem here is clearly that outside of the module interface it's not known which event types needed to be handled by the module.

It depends on the callback, if you're talking about check_event_for_spam, then the module is expected to handle all events (where by "handle" I also mean drop them for types it's not interested in).

@babolivier
Copy link
Contributor

After discussing this internally it looks like the team's opinion is that we should fail the request if the module encounters an error. However, note that check_event_for_spam currently doesn't have this kind of error handling/protection, so I'm a bit unsure why this is showing up.

@erikjohnston
Copy link
Member

Yeah, a quick look at the code suggests if check_event_for_spam fails by raising an exception the event should be rejected. Is that what they mean here?

@erikjohnston erikjohnston added the X-Needs-Info This issue is blocked awaiting information from the reporter label Aug 23, 2021
@DMRobertson
Copy link
Contributor

Is this related to #11031 ?

@benparsons
Copy link
Member Author

@DMRobertson yes, I think O's issue clarifies and replaces my own.

@DMRobertson DMRobertson removed the X-Needs-Info This issue is blocked awaiting information from the reporter label Oct 19, 2021
@DMRobertson
Copy link
Contributor

Thanks! Let's close this and continue the discussion there.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants