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

delayable attribute lost when the @job decorator is not added on base method #273

Closed
guewen opened this issue Oct 29, 2020 · 5 comments · Fixed by #274
Closed

delayable attribute lost when the @job decorator is not added on base method #273

guewen opened this issue Oct 29, 2020 · 5 comments · Fixed by #274

Comments

@guewen
Copy link
Member

guewen commented Oct 29, 2020

Example of error message:

AttributeError: method cron_actions on stock.buffer(5577,) is not allowed to be delayed, it should be decorated with odoo.addons.queue_job.job.job

If I have such dependencies:

module_a
  |- module_b
  |- module_c

module_b and module_c have no dependencies on each other.

In module_a, we have a method StockMove._update_foo.
In module_b, we add @job on StockMove._update_foo.
In module_c, we have another override of StockMove._update_foo, without decorator.

It seems that depending on the loading order, we'll lose the properties added by @job.

AFAIU, it happens because of the way the registry builds a new instance based on a recomposed dependency graph, we can't be sure the kept function will be the one which has been decorated and has the delayable attribute. We would need to propagate it.

That being said, the delayable attribute is there only to check that we decorated the method with @job. If we remove the check at

queue/queue_job/job.py

Lines 81 to 85 in d465bdd

if not getattr(recordset_method, "delayable", None):
raise AttributeError(
"method %s on %s is not allowed to be delayed, "
"it should be decorated with odoo.addons.queue_job.job.job"
% (name, self.recordset)
, the enqueuing works perfectly.

We can consider removing the need for @job (make it optional with default values). That would imply:

  • stop storing the @job attributes on the method (

    queue/queue_job/job.py

    Lines 828 to 831 in d465bdd

    func.delayable = True
    func.delay = delay_func
    func.retry_pattern = retry_pattern
    func.default_channel = default_channel
    ): delayable could be removed, delay is deprecated, remains retry_pattern and default_channel to store elsewhere, best place would be in queue.job.function I think as we could change them from the UI
  • same for the related actions attrs...

    queue/queue_job/job.py

    Lines 897 to 898 in d465bdd

    func.related_action = action
    func.kwargs = kwargs
  • think about the security implications by not having the @job allow list
  • delayable is used to find the job methods used to create queue.job.function (
    def _register_hook(self):
    """Register marked jobs"""
    super(Base, self)._register_hook()
    job_methods = [
    method
    for __, method in inspect.getmembers(
    self.__class__, predicate=inspect.isfunction
    )
    if getattr(method, "delayable", None)
    ]
    for job_method in job_methods:
    self.env["queue.job.function"]._register_job(self, job_method)
    ), have to think if we can do it another way (create queue.job.function the first time it's delayed? but then we don't have the ability to put default options...)

Originally posted by @guewen in #270 (comment)

@guewen
Copy link
Member Author

guewen commented Oct 29, 2020

* (create `queue.job.function` the first time it's delayed? but then we don't have the ability to put default options...)

Well, this could be replaced by explicit XML records, and if you use with_delay() on a job which doesn't have a queue.job.function record, then you have the default options...

@lmignon
Copy link
Contributor

lmignon commented Oct 30, 2020

On one side, the @job decorator is an elegant / visual / and pythonic way to mark a method to support asynchronous calls.
On the other side the use of XML records of queue.job.function to declare default parameter for delayable methods could provide a more easy way to override/adapt these default parameters to the client context and reduce the complexity of the job methods registration process.
Last but not least, with this change every methods becomes delayable by default.
Why not.

@simahawk
Copy link
Contributor

Last but not least, with this change every methods becomes delayable by default.

This! This is a side effect which adds a lot of value IMO.

Nevertheless, if mod A adds the function, and B and C override it: who wins? You still have the possibility of conflicts between settings. No?

@lmignon
Copy link
Contributor

lmignon commented Oct 30, 2020

Last but not least, with this change every methods becomes delayable by default.

This! This is a side effect which adds a lot of value IMO.

👍

Nevertheless, if mod A adds the function, and B and C override it: who wins? You still have the possibility of conflicts between settings. No?

In such a case, all the overrides are into the call stack but the order into the stack is random for b and c ...

module_a
  |- module_b
  |- module_c

c -> b -> a
or
b -> c -> a

@guewen
Copy link
Member Author

guewen commented Oct 30, 2020

On the other side the use of XML records of queue.job.function to declare default parameter for delayable methods could provide a more easy way to override/adapt these default parameters to the client context and reduce the complexity of the job methods registration process.

Yes, it opens many possibilities and indeed simplify a lot the registration. Not saying that declaring the XML records is optional so in the simplest cases, you only need to use with_delay() and it works with default settings.

You still have the possibility of conflicts between settings. No?

If you mean 2 modules can create the same xml record for queue.job.function, yes you are correct. But it's manageable in some ways (magically merge them, not sure if it works properly on uninstall... or take the first one and let users handle duplicates).

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 a pull request may close this issue.

3 participants