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

Result middleware should read message options first #591

Closed
7 tasks done
huwylphimet opened this issue Nov 21, 2023 · 2 comments
Closed
7 tasks done

Result middleware should read message options first #591

huwylphimet opened this issue Nov 21, 2023 · 2 comments

Comments

@huwylphimet
Copy link
Contributor

huwylphimet commented Nov 21, 2023

Issues

Checklist

  • Does your title concisely summarize the problem?
  • Did you include a minimal, reproducible example?
  • What OS are you using?
  • What version of Dramatiq are you using?
  • What did you do?
  • What did you expect would happen?
  • What happened?

What OS are you using?

macOS 13.5.2

What version of Dramatiq are you using?

1.15.0

What did you do?

implemented an actor with the Result middleware and passing the actor options (store_results and result_ttl) within the message sent

What did you expect would happen?

  1. I would expect that the result_ttl equals the value from the option stored within the message
  2. I would expect that the warning created in after_process_message() uses the store_results option from the message

What happened?

  1. the result_ttl within the message option was ignored and only the option value from the actor and Result middleware instance or DEFAULT_RESULT_TTL = 600000 was used
  2. the following warning was created even if store_results was set to False within the message options

Actor '%s' returned a value that is not None, but you haven't set its store_results' option to True' so the value has been discarded." % message.actor_name

Example

import dramatiq

from dramatiq.brokers.redis import RedisBroker
from dramatiq.results.backends import RedisBackend
from dramatiq.results import Results, ResultTimeout, ResultMissing
from dramatiq.middleware import CurrentMessage

broker = RedisBroker()
result_backend = RedisBackend(client=broker.client)
broker.add_middleware(Results(backend=result_backend))
broker.add_middleware(CurrentMessage())
dramatiq.set_broker(broker)


@dramatiq.actor(store_results=True)
def add(x, y):
    message = CurrentMessage.get_current_message()
    store_results = message.options.get("store_results", True)
    print(f"message 'store_results' option is '{store_results}'")
    return x + y if store_results else None


if __name__ == "__main__":
    message = add.send_with_options(
        args=(1, 2),
        store_results=False)
    try:
        print(message.get_result(backend=result_backend, block=True, timeout=5000))
    except (ResultTimeout, ResultMissing):
        print("result not stored")

Fix proposition

a simple fix could be to slightly modify _lookup_options() in dramatiq/results/middleware.py on lines 86-87

def _lookup_options(self, broker, message):
    try:
        actor = broker.get_actor(message.actor_name)
        store_results = actor.options.get("store_results", self.store_results)  #line 86
        result_ttl = actor.options.get("result_ttl", self.result_ttl)  #line 87
        return store_results, result_ttl
    except ActorNotFound:
        return False, 0

replace lines 86-87 with

store_results = message.options.get("store_results", actor.options.get("store_results", self.store_results))
result_ttl = message.options.get("result_ttl", actor.options.get("result_ttl", self.result_ttl))
@huwylphimet
Copy link
Contributor Author

A new version has been published since the creation of this issue but I have not yet received any feedback regarding my proposal.
Therefore I've created the following PR accordingly: #612
Please consider merging my PR or commenting on this issue.
Thanks

@huwylphimet
Copy link
Contributor Author

This issue has been fixed by merging the PR #612 and has been released with v1.17.0

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

No branches or pull requests

1 participant