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

Depend less on Celery #4

Open
marten-de-vries opened this issue Feb 16, 2019 · 6 comments
Open

Depend less on Celery #4

marten-de-vries opened this issue Feb 16, 2019 · 6 comments

Comments

@marten-de-vries
Copy link
Owner

The hub currently schedules a separate celery job for each HTTP request that it sends out during content delivery. That does not scale, especially with a large message body. A better approach would be to store the content in the database, and keep track of the last delivery for each subscription. Then a single background task can handle the content delivery.

@marten-de-vries
Copy link
Owner Author

The current way of handling multiple hubs works, but it's a bit of a hack. It would be better to define the tasks only once. The overhead of passing in hub information with each job is probably worth it (especially after the changes above reduce the amount of required jobs).

@dannmartens
Copy link

I've created a proof-of-concept which removes Celery as a run-time dependency.

Instead, I've created a stub 'Celeriac' which is API-compatible with Celery, but simply uses an asyncio event loop in a separate thread.

As the main code depends heavily on the Celery API, it was a bit of an undertaking to stub out unnecessary calls and replace needed calls with an asyncio equivalent.

Of course, by doing it this way, I haven't introduced a proper abstraction facade yet: it's all still a bit "celery" (pun intended).

@marten-de-vries
Copy link
Owner Author

Sounds interesting! Being able to swap out Celery for something else altogether is a bit wider in scope than the issue discussed here, but I would love for Flask-WebSub to support that.

@dannmartens
Copy link

All the tests pass, but I already have butchered the pytest fixtures and simplified the tests, mainly because the TestApp code and the dependency on app _state and Celery workers are not necessary for Celeriac to function. Fortunately, I still have an old baseline with the test stubbing, from the time when I was still using the original testing code. Regarding that test stubbing, it suffices to say "pretty it ain't."

I will go through the WebSub.rocks tests, next week, to find any remaining issues. It would be lovely to see this code end up in your package, and ultimately in PyPI.

Also worth noting is that Werkzeug has changed its API, and the current baseline is not compatible with those changes.

image

@dannmartens
Copy link

Okay,

I have completed the WebSub.rocks Hub tests, successfully.

image

However, I had to patch the hub/tasks.py code, as it doesn't seem to build up the request headers for a notification in a clean way.

The following statement copies in a lot of headers, which do not belong in the notification request:

27:   headers = updated_content['headers']

As a result, the WebSub.rocks server refuses the notification, on account of those stray headers:

 ERR 2020-10-07 10:08:40,059 [DEBUG] urllib3.connectionpool - Starting new HTTPS connection (1): websub.rocks:443
 ERR 2020-10-07 10:08:40,606 [DEBUG] urllib3.connectionpool - https://websub.rocks:443 "POST /hub/102/sub/Rhzj6gUYqw9mcRDdTVaK HTTP/1.1" 400 173
 OUT Debug! Server replied with: code=400 response.text=
 OUT <html>
 OUT <head><title>400 Bad Request</title></head>
 OUT <body bgcolor="white">
 OUT <center><h1>400 Bad Request</h1></center>
 OUT <hr><center>nginx/1.14.2</center>
 OUT </body>
 OUT </html>
 ERR 2020-10-07 10:08:40,608 [WARNING] websub - Notification failed

I still need to stub out the Celery retry code, which I do not support yet (this became visible with the above tests).

@marten-de-vries
Copy link
Owner Author

Weird, that's never been a problem before.

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

2 participants