Skip to content
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

Don't allow Result middleware without a result backend. #332

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

AndreCimander
Copy link
Contributor

Since I saw some issues related to the result middleware and django_dramatiq lately, I think it might be sensible to be more strict and descriptive when using the result middleware the wrong way.

My first idea was to use a fallback call like get_broker, but this would also require changes to django_dramatiq which needs to set the backend first, but it's currently way too hot to be really productive and motivated, so raising an descriptive error was my next idea 🙃

Feel free to adjust the error message, on a second read it sounds a bit harsh.

Also, point django_dramatiq users towards the right direction how to setup the result backend.
@Bogdanp
Copy link
Owner

Bogdanp commented Aug 10, 2020

I'm reluctant to make this change since it's technically breaking and, if we were to go in this direction, then I'd rather just make the argument required than have it be optional and error out when it's None. I'll have to think about this some more.

@AndreCimander
Copy link
Contributor Author

Yeah, I have also thought about making the backend parameter required, since the middleware is technically broken without a backend, but you'll lose the ability to give a more specific error message, eventually leading to more confusion for django_dramatiq users.

Maybe just changing django_dramatiq's behaviour to automatically inject the backend into any result-middlewares without a backend is the better approach, but this still has the potential to lead to a broken middleware state if one adds the middleware manually without a backend 🤔

@Bogdanp Bogdanp added this to the v2.0.0 milestone Oct 18, 2020
@jenstroeger
Copy link
Contributor

I’ve been thinking about a similar/related scenario: in a codebase I worked with there are oodles of actor functions, some of which return values and some don’t. Without a result backend Dramatiq’s worker issues a warning:

dramatiq/dramatiq/worker.py

Lines 478 to 486 in 8dd6a75

if res is not None \
and message.options.get("pipe_target") is None \
and not has_results_middleware(self.broker):
self.logger.warning(
"Actor '%s' returned a value that is not None, and you haven't added the "
"Results middleware to the broker, so the value has been discarded. "
"Consider adding the Results middleware to your broker or piping the "
"result into another actor." % actor.actor_name
)

Which made me wonder: would you be open to a “NullBackend” which simply ignores results altogether? A bit like the StubBackend. Then we’d (temporarily) add the middleware and drop the result values, maybe log the actor function that attempted to return a result. The NullBackend would be a nice addition to the Dramatiq result backends?

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

Successfully merging this pull request may close these issues.

3 participants