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

DEP 0009: Async-capable Django #56

Merged
merged 6 commits into from
Jun 1, 2019
Merged

Conversation

andrewgodwin
Copy link
Member

@andrewgodwin andrewgodwin commented May 9, 2019

This is a pull request for this first draft of the Async DEP (currently numbered 0009 unless another lands first).

This is an unusual DEP as it has a very broad scope, and is very long; I do not expect this to be merged straight away, but instead for this PR to serve as a central point for specific patches and updates as we revise it initially.

This DEP is also incredibly long, at around 7,000 words, so I have included a very high-level summary at the start of it. If you read nothing else, read that and the foreword.

If you wish to read the DEP, it may be nicer to use the rich text display on GitHub at https://github.com/andrewgodwin/deps/blob/async/draft/0009-async.rst

The mailing list discussion about this is here: https://groups.google.com/forum/#!topic/django-developers/5CVsR9FSqmg

@avanov
Copy link

avanov commented May 10, 2019

Hi @andrewgodwin, thanks for this work.

I read it yesterday and I have a few points that I'd like to be addressed in this draft before it's made official:

• I saw that parallelism is mentioned in a couple of sections of the current version of the draft (for instance https://github.com/andrewgodwin/deps/blob/async/draft/0009-async.rst#id25). Yet asyncio is concurrent and is not parallel. The distinction is important for further discussion, because async-capable Django exists already in other forms, without utilizing async/await syntax, nameley in gevent-/eventlet-based setups;

• Django could utilize gevent.spawn() with a combination of Groups and Pools under its API for finer-granular concurrent async database API to achieve the same goal described in this proposal, so there’s a room for improvement in the domain of concurrency that doesn't depend on async/await syntax;

• the same is applicable to handling websocket connections either within the same process or through channels - a gevent-aware webserver is capable of handling both http and ws traffic concurrently, and granularity is adjustable;

gevent and alike are not mentioned in the list of alternatives https://github.com/andrewgodwin/deps/blob/async/draft/0009-async.rst#alternatives and the current version of the draft only mentions other async frameworks that utilize async/await syntax...

• ...hence the conclusion that

The popularity of asyncio-based libraries, though, makes it the only viable choice

seems a bit too biased at the moment, especially because risks and ramifications of the transition are not outlined clearly:

  • a tradeoff between latency and throughput;
  • "stickiness" of async/await syntax that mandates more sophisticated API design approaches.

@andrewgodwin
Copy link
Member Author

@avanov I have updated the DEP to use "concurrent" rather than "parallel" where it makes sense. Thanks for picking that up.

I have also added a section that explains why we are not choosing gevent. I have used gevent on several large projects and ended up regretting it every time, sadly; the combination of monkeypatching and unpredictable context-switching made it very hard to write safe, stable code in a large codebase. Working out what's patched, debugging through greenlets, and making C extension-based code compatible are all difficult.

I understand that there's a decent community of people who use gevent, but much like Curio, it ends up being too small relative to the growing number of asyncio libraries to make it viable for a large, volunteer project.

draft/0009-async.rst Outdated Show resolved Hide resolved
draft/0009-async.rst Outdated Show resolved Hide resolved
draft/0009-async.rst Outdated Show resolved Hide resolved
draft/0009-async.rst Outdated Show resolved Hide resolved
draft/0009-async.rst Outdated Show resolved Hide resolved
draft/0009-async.rst Outdated Show resolved Hide resolved
Copy link

@rixx rixx left a comment

Choose a reason for hiding this comment

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

The changes you pushed make many things much clearer – thank you for the added discussion of performance, and the potential impact of abandoning the implementation.

with new database connections. Any transactions that were running outside the
block continue; any ORM calls inside the block operate on a new database
connection and will see the database from that perspective. If the database
has transaction islation enabled, as most do by default, this means that the
Copy link

Choose a reason for hiding this comment

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

Suggested change
has transaction islation enabled, as most do by default, this means that the
has transaction isolation enabled, as most do by default, this means that the

This paragraph is much more understandable now, thank you!

@andrewgodwin andrewgodwin marked this pull request as ready for review June 1, 2019 17:45
@andrewgodwin andrewgodwin merged commit 438226e into django:master Jun 1, 2019
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