Fix DatabaseSyncToAsync

[blueyed]

• copy/use connections from the main thread, which are in an atomic
  block (used in tests)
• do not close connections in atomic blocks

Ref: #1091 (comment)

[auvipy]

fix lint pease!

[blueyed]

@auvipy
Fixed.
Maybe you want to review the code? :)

[auvipy]

yes of course, but should the errors of most of the python version is failing... I am not familiar with the error messages, could you have another look?

[blueyed]

initializer is Python 3.7+ only.
So if this approach is taken (after feedback via #1091 (comment)), a method for backward compatibility would be required.

[auvipy]

seems good.

[blueyed]

I've moved the call to _inherit_main_thread_connections to thread_handler, not requiring an initializer.

And then to its own class, since it seems to be only relevant for tests after all?!

[carltongibson]

Hey @blueyed. Fancy rebasing this after #1291? It would give me something to look at on Travis. 😀

[carltongibson]

Right, so this probably needs a note in the testing topics doc. (And probably a cross-link from the databases topic too, whilst we're there...)

[carltongibson]

Right, so two questions:

1. If I'm using database_sync_to_async() in my code, which I'm then testing, is this going to help?
2. As a testing tool, wouldn't this be better off in pytest-django?

[blueyed]

1. If I'm using database_sync_to_async() in my code, which I'm then testing, is this going to help?

You can use a conditional import based on settings.TESTING or similar. Also monkeypatching during tests would work.

2. As a testing tool, wouldn't this be better off in pytest-django?

Well, maybe - but it affects test runners in general, not just pytest. Therefore it makes sense to ship it with channels.

[blueyed]

JFI: I would like to get feedback from @adamhooper (via #1091 (comment)) before doing anything here.

[blueyed]

Also I consider this to be a hack really, and there is hopefully a better way.
Just came across the following note in Django's Atomic: https://github.com/django/django/blob/bf9e0e342da3ed2f74ee0ec34e75bdcbedde40a9/django/db/transaction.py#L159

[blueyed]

Added tests.

There are two issues here: with conn_max_age=0 (the default) it does not run into "sqlite3.OperationalError: database table is locked" or postgresql not being able to teardown the db, but it will not rollback the changes (due to in_atomic_block being reset).
So there are 4 variants of the test testing this.

[blueyed]

btw: just found this: https://github.com/blueyed/asgiref/blob/1c23eb6832b39e94628fd7600c0bc1baf3105dbe/asgiref/local.py#L22-L27

I could not grasp how/where it is being used though.
Would channels have to use it instead of threading.local here? https://github.com/blueyed/asgiref/blob/1c23eb6832b39e94628fd7600c0bc1baf3105dbe/asgiref/sync.py#L117 (added in django/asgiref@768a071) /cc @andrewgodwin

[blueyed]

I've removed DatabaseSyncToAsyncForTests, fixing DatabaseSyncToAsync itself.
After all it might also affect real applications when using atomic blocks.
Additionally it would require to e.g. monkeypatch users of the non-test decorator. (I've been using a conditional import for when (not) running tests for the app itself)

[carltongibson]

Okay, thanks @blueyed. I shall have a look at this (finally) over the next period.

[adamhooper]

As a user of Django-Channels, this complexity scares me, outside of a unit-test framework.

@blueyed: outside of unit tests, how can a main-thread database connection enter an atomic block? My understanding is that the main thread should never connect to the database, period. (An exception is Django-Channels unit tests, evidently.)

Django saved my neck while I was first delving into Django-Channels: it raised a DatabaseError when I accessed a connection from the wrong thread. That error is a feature, not a bug. I don't think Django-Channels should risk its users losing this valuable DatabaseError just so Django-Channels unit tests can pass.

I see considerable risk. For one thing, your implementation never calls dec_thread_sharing(). This is probably a bug, and I see it as evidence that This Stuff Is Scary.

More broadly, imagine a user accidentally uses the main-thread connection. (In my experience, this is a very common error.) Currently, that erroneous main-thread connection won't cycle like it should: the user will get a "lost connection" error once in a while, but the site will stay up. But with the code in this pull request, any database operation that starts while the (buggy) main-thread connection is in an atomic block will replace the worker thread's database connection with the main-thread connection, irreversibly. Now that two threads are using the same database connection, that doubles the risk of the problem spreading to another thread. Soon, all worker threads will use the main-thread database connection. This produces undefined behavior. Ironically, this is exactly what DatabaseError is meant to alert us to ... only, inc_thread_sharing() prevents us from knowing.

I'm not a Channels maintainer; but could my concerns please be addressed? I'd be much more comfortable if this code -- which seems specially made to help Django-Channels unit tests pass -- were to part of the unit test framework. Shouldn't DatabaseSyncToAsync be left alone?

[adamhooper]

(Also: what happens if a Python module that decorates a function with @database_sync_to_async is imported from a thread that isn't the main thread? Then its so-called "main" connection will actually be the thread's connection. Cue the disaster scenario from my previous point....)

(I understand that async atomic blocks would be nice in Django Channels. But this pull request doesn't support async atomic blocks. As far as I can tell, this pull request is just to make unit tests pass.)

[blueyed]

@adamhooper
Thanks for looking into this.
I am not really familiar with it anymore currently, but am using it still (if I am not mistaken completely). See also django/asgiref#88 / #1091 (comment).

outside of unit tests, how can a main-thread database connection enter an atomic block?

It is for tests only, yeah (IIRC).
Another separate issue might be https://code.djangoproject.com/ticket/30448 / django/django#11769 still (_close_old_connections / close_if_unusable_or_obsolete), not sure really currently anymore.

For one thing, your implementation never calls dec_thread_sharing(). This is probably a bug, and I see it as evidence that This Stuff Is Scary.

I agree that it is scary - not sure about dec_thread_sharing though - it should probably not be called as long as the connection is open I guess? But it might be a bug.

[blueyed]

This does not work on Django 3.0 anymore, when database_sync_to_async is used from inside of an async def. Pushed a fix for the test to work around this.
Due to django/django@a415ce7 (django/django#11209).
/cc @andrewgodwin

[adamchainz]

I just looked at this bug in depth and wrote my findings in this comment: #1091 (comment)

I think much of this PR is not really relevant any more:

1. The connection sharing between threads should no longer be necessary after asgiref 3.5.1, which causes sync_to_async functions to run on the main thread in tests, reusing the same connection as used for TestCase's atomics.
2. Using postgresql to trigger the failure is not required - we only need use SQLite with a non-in-memory database (see my repro repo.
3. Overriding close_old_connections to check if not conn.in_atomic_block: would work, but I think it would be better to disable calling close_old_connections() during tests altogether.

Thank you for digging in here @blueyed . It seems the fix in asgiref has changed the nature of this bug slightly, and should make it easier for us to write a fix.