Skip to content

Conversation

@TheTechromancer
Copy link
Contributor

@TheTechromancer TheTechromancer commented Mar 12, 2025

Hi all, thanks for making this repo; it's really useful.

This PR fixes some typos and also adds a warning when a scheduled task's broker doesn't match the broker it's being executed on. This has fucked me over several times when scheduled tasks are registered but silently fail to execute. This was my own fault since I was double-registering tasks. But it's a nasty pitfall.

Should we detect mismatched brokers a bit sooner, i.e. when the task is first registered? Probably if a user tries to register a function that's already an instance of a decorated task, we should throw an error if the brokers don't match.

@s3rius
Copy link
Member

s3rius commented Mar 13, 2025

Yooooo. That's really bad. Sorry for not thinking of this case. I should have created this warning earlier.

This PR is amazing. Thanks for making it!

s3rius
s3rius previously approved these changes Mar 13, 2025
Copy link
Member

@s3rius s3rius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes should fix the error.

@TheTechromancer
Copy link
Contributor Author

Thanks @s3rius, I added one more check during task registration, which will throw a TaskRejectedError if the task is already assigned to a different broker. This should never trigger under normal circumstances.

taskiq/taskiq/abc/broker.py

Lines 510 to 530 in 9f5cf43

def _register_task(
self,
task_name: str,
task: AsyncTaskiqDecoratedTask[Any, Any],
) -> None:
"""
Method is used to register tasks.
By default we register tasks in local task registry.
But this behaviour can be changed in subclasses.
This method may raise TaskRejectedError if task has already been registered to a different broker.
:param task_name: Name of a task.
:param task: Decorated task.
"""
if task.broker != self:
raise TaskRejectedError(
f"Task already has a different broker ({task.broker})",
)
self.local_task_registry[task_name] = task

@s3rius
Copy link
Member

s3rius commented Mar 13, 2025

Please run pre-commit run -a locally to fix most of style issues.

https://taskiq-python.github.io/contrib.html

@TheTechromancer
Copy link
Contributor Author

TheTechromancer commented Mar 13, 2025

There are some weird things going on, where certain linters break the syntax and cause subsequent linters to fail 😬

Specifically it looks like add_trailing_commas is adding them in unwanted places and creating invalid python syntax

:param task: Decorated task.
"""
if task.broker != self:
raise TaskRejectedError(
Copy link
Member

@s3rius s3rius Mar 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not a syntax error. It's izulu. We use it for errors.

Please define separate error class for this error with

__template__ and broker argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rgr. This is the syntax error:

image

image

@s3rius
Copy link
Member

s3rius commented Mar 25, 2025

Okay. Actually this PR is fine by me. So, I'm gonna merge it an fix on the main branch.

@s3rius s3rius merged commit 91d7d65 into taskiq-python:master Mar 25, 2025
30 of 33 checks passed
s3rius added a commit that referenced this pull request Apr 4, 2025
* fix typos

* warn if task broker doesn't match self

* oopsie

* Update taskiq/schedule_sources/label_based.py

Co-authored-by: Pavel Kirilin <[email protected]>

* Update taskiq/schedule_sources/label_based.py

Co-authored-by: Pavel Kirilin <[email protected]>

* raise error on broker mismatch

* ruffed

* create new error class

---------

Co-authored-by: Pavel Kirilin <[email protected]>
s3rius added a commit that referenced this pull request Apr 4, 2025
* fix typos

* warn if task broker doesn't match self

* oopsie

* Update taskiq/schedule_sources/label_based.py

Co-authored-by: Pavel Kirilin <[email protected]>

* Update taskiq/schedule_sources/label_based.py

Co-authored-by: Pavel Kirilin <[email protected]>

* raise error on broker mismatch

* ruffed

* create new error class

---------

Co-authored-by: Pavel Kirilin <[email protected]>
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.

2 participants