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

Polling publishers #51

Merged
merged 27 commits into from Jun 29, 2021
Merged

Polling publishers #51

merged 27 commits into from Jun 29, 2021

Conversation

Tijani-Dia
Copy link
Collaborator

@Tijani-Dia Tijani-Dia commented Jun 25, 2021

This PR adds a PollingPublisherMixin for publishers using the polling technique.

It also adds an IntervalPollingPublisher and a LongPollingPublisher.

I haven't added tests yet, I want to have a feedback on the design first.
To try it out with the testapp, checkout this branch (polling-publisher).

Here are the steps to take if you want to test the interval polling publisher:

  1. Add this to your settings.py file:
    WAGTAIL_LIVE_PUBLISHER = "wagtail_live.publishers.IntervalPollingPublisher"

  2. Add wagtail_live urls in tests.urls like this:

from wagtail_live import urls as live_urls

urlpatterns += [
    path("wagtail_live/", include(live_urls)),
]
  1. Create a template for BlogPage model and add the following:
{% include "wagtail_live/live_posts.html" %}

{% include "wagtail_live/polling/interval_polling.html" %}
  1. Create a live page in the admin interface with channel_id=test_channel for example

  2. Head to http://127.0.0.1:8000/wagtail_live_interface/channels and create a channel with the same name as the BlogPage created, in this example test_channel

  3. (Optional) You can also add a WAGTAIL_LIVE_POLLING_INTERVAL value in milliseconds in your settings.

We need to set DEBUG to True in order to get static files served.
And it should be fine.

The steps to take for the long polling publisher are similar with these differences:

  1. WAGTAIL_LIVE_PUBLISHER = "wagtail_live.publishers.LongPollingPublisher"

  2. Create a template for BlogPage model and add the following:

{% include "wagtail_live/live_posts.html" %}

{% include "wagtail_live/polling/long_polling.html" %}
  1. (Optional) You can add a WAGTAIL_LIVE_POLLING_TIMEOUT value in seconds in your settings.

Additional remarks

From django docs, long polling is a good candidate for async views:

Under a WSGI server, async views will run in their own, one-off event loop. This means you can use async features, like concurrent async HTTP requests, without any issues, but you will not get the benefits of an async stack.

The main benefits are the ability to service hundreds of connections without using Python threads. This allows you to use slow streaming, long-polling, and other exciting response types.

I tried to make an AsyncLongPollingPublisher but I don't think it changes a lot under WSGI but maybe we can add it for apps running on ASGI?

Copy link
Collaborator

@lucasmoeskops lucasmoeskops left a comment

Choose a reason for hiding this comment

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

Nice work!

tests/wagtail_live/publishers/test_longpollingpublisher.py Outdated Show resolved Hide resolved
src/wagtail_live/static/wagtail_live/js/intervalpolling.js Outdated Show resolved Hide resolved
src/wagtail_live/publishers.py Outdated Show resolved Hide resolved
polling_timeout = get_polling_timeout()

while time.time() - starting_time < polling_timeout:
live_page = get_object_or_404(self.model, channel_id=channel_id)
Copy link
Collaborator

Choose a reason for hiding this comment

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

At some point this might be optimized since when many people watch the live-stream they will essentially all be doing many identical queries to the database all the time. But database caching like django-cachalot will already help out in cases like this I think (only when you have considerably many users).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Alright, do we support it out of the box and define a boolean setting like WAGTAIL_LIVE_USE_CACHALOT or shall we just mention in the docs that this optimisation is possible?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should just mention this possibility. If people use e.g. cachalot it will be automatically fixed, so no setting is required.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good👍

(float) timestamp of the last update received in the client side.
"""

return float(request.GET.get("last_update_ts"))
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: what if the request doesn't have a last_update_ts parameter? Should we return something else instead? Maybe None but code that calls this method would have to handle that as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that this can happen only if page.latest_revision_created_at is None. And if that happens, we will be in trouble sooner since we call the timestamp method on None object here. So, we need to handle it from there I think. How do you suggest to handle it?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, ok. Maybe we shouldn't worry too much and just return a 400 bad request response to the client if it doesn't include a last_update_ts parameter.

@Stormheg Stormheg merged commit 4c71e64 into main Jun 29, 2021
@Tijani-Dia Tijani-Dia deleted the polling-publisher branch June 29, 2021 12:36
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.

None yet

3 participants