Skip to content

Refactor code base to add ability for multiple concurrency models / executors #19

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

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

hballard
Copy link
Owner

Initial executors will include gevent, asyncio, and django channels

hballard added 3 commits June 24, 2017 01:53
Set default executor as gevent since it works on both Python 2/3; both
the manager and transport packages allow user to pass in an executor
class at instantiation; may modify this user interface once I add
asyncio as an executor.
hballard added 12 commits June 25, 2017 00:19
Shares essentially same interface as GeventExecutor
Passes all existing tests.  All concurrency and websocket code now uses
GeventExecutor class, which is injected (vs inherited like previous) by
user at Pubsub instantiation. I will modify AsyncioExecutor to
now utilize the same interface as the GeventExecutor.
Also refactored project layout a bit more.
Gevent has one additional test failing after refactor; need to
troubleshoot it before I release.
Change asyncio "join" method; construct "delayed_backgrd_task" method;
various other minor changes
This is versus the previous implementation of using static methods on
the executor and passing it explicitly to each one
Continue to work on getting transport server tests to pass for
asyncio.  There are two gevent transport server tests that need to
be fixed.  Also refactored out fakeredis, preferring to start
redis-server instance in test setup code
@Thibaut-Fatus
Copy link

Well done! Do you need some help for the django-channels executor?

Several asyncio server class tests are the only tests still failing after
this major refactor. Focus on getting these to pass.
@Thibaut-Fatus
Copy link

We've been looking into using django-channels as executor, and even if the use of channels as WS transport medium seemed quite straightforward, replicating your threading / asynchrone code for channels feels weird since they already use a event queue and thus are non blocking, Any thoughts on this ?

@hballard
Copy link
Owner Author

hballard commented Aug 2, 2017

Can you give me a specific example? I'm not super familiar w/ django channels...so that may very well be. I was going to have to play around w/ it once I got to that point to see what was possibly. But, if you can give me an example of the specific threading / async code you are referring to, then I'd be happy to offer any advice I have. If you are talking about the keep_alive_timer function or the background task that checks the redis-pubsub for messages, then I was thinking to try and use a custom django consumer (with maybe an os thread?).

Honestly, it might be that we need to have separate subscription manager, pubsub or server classes, beyond just a new executor class (maybe a mixin of some sort on top of a base class). I'm actually struggling w/ the asyncio integration a bit right now in trying to get the subscription server tests to pass, using just a new executor. The need to have async def coroutines in order to be able to await / yield from a code block really makes it hard to not duplicate code logic when integrating asyncio...which I was really not wanting to do. In some case I can get away with just scheduling a coroutine into a task using asyncio.ensure_future() at the very end of a function, but in other scenarios the control flow really demands that I'm able to await that block of code. I suppose I could also attach a callback to the returned future, but again, it means duplicating code.

Also, did you happen to see this post from syrusakbary over the weekend. This would take care of the subscription manager and pubsub classes I suppose, but there would still need to be separate integration / plugin for each async framework integrating the websocket / concurrency model (django, gevent, asyncio, etc.).

@Thibaut-Fatus
Copy link

Seems like he will integrate django-channels : 'websockets with django-daphne'

In your implementation, in order to achieve non-blocking code, you use asyncio or gevent, but django-channels have their own PubSub mechanism and thus I think it would be quite different from your architecture. In fact using Channels we have no access to the underlying ws, the one needed as argument when you create your SubscriptionServer

@hballard
Copy link
Owner Author

hballard commented Aug 2, 2017

I just re-read his post and I agree...sounds like he will integrate them. I was still playing around w/ this asyncio refactor, because I had already started it and it gave me a chance to explore asyncio (which I had really not done before), but after seeing that post this weekend, I've been thinking I would just wait for graphene 2.0 and maybe see if I can contribute to graphql-core or graphene, instead of continuing to evolve this implementation.

@tricoder42
Copy link

Here's a gist with my solution using django-channels. It's a working proof of concept. Next task is optimize it for production environment. Feedback welcome!

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.

3 participants