Idle database connections not reaped before database_sync_to_async code

[adamhooper]

This has the same effects (and solution) as #871

Steps to reproduce:

1. Run a @database_sync_to_async function
2. Let CONN_MAX_AGE expire
3. Have the database drop the connection
4. Run a @database_sync_to_async function in the same thread as in step 1

Expected results: In step 4, Channels reconnects
Actual results: In step 4, Channels tries to use the expired connection

Obvious fix: make @database_sync_to_async call close_old_connections() before calling the inner function.

To reproduce:

import asyncio
import os
import unittest
from channels.db import database_sync_to_async
from django.db import connection, close_old_connections
from django.conf import settings


if settings.DATABASES['default']['CONN_MAX_AGE'] != 1:
    raise AssertionError(
        "Please set settings.DATABASES['default']['CONN_MAX_AGE'] to 1. "
        'This test needs the connection to die.'
    )

os.environ['ASGI_THREADS'] = '1'
if os.environ.get('ASGI_THREADS') != '1':
    raise AssertionError(
        'Please set the environment variable ASGI_THREADS=1. '
        'This test depends on Channels running two queries on the same '
        'database connection.'
    )


@database_sync_to_async
def ping_database():
    # Intended behavior: close_old_connections() before running this code
    # close_old_connections()
    with connection.cursor() as cursor:
        cursor.execute('SELECT 1')



class BugTest(unittest.TestCase):
    def test_break_db(self):
        async def go():
            print('Pinging database...')
            await ping_database()
            input(
                'Now, go destroy the database connection somehow '
                '(for instance, by dropping it on the server side) '
                'and press Enter:'
            )
            print('Pinging database again...')
            await ping_database()
            print('Whew -- connection is still alive')

        loop = asyncio.get_event_loop()
        loop.run_until_complete(go())

... deleting the connection (Postgres, in my case) when prompted:

> SELECT pg_terminate_backend((SELECT pid FROM pg_stat_activity WHERE query = 'SELECT 1'));

... and after pressing Enter in the test, I see:

System check identified no issues (0 silenced).
Pinging database...
Now, go destroy the database connection somehow (for instance, by dropping it on the server side) and press Enter:
Pinging database again...
E
======================================================================
ERROR: test_break_db (server.tests.test_bug.BugTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/root/.local/share/virtualenvs/app-4PlAip0Q/lib/python3.6/site-packages/django/db/backends/utils.py", line 62, in execute
    return self.cursor.execute(sql)
psycopg2.OperationalError: terminating connection due to administrator command
server closed the connection unexpectedly
        This probably means the server terminated abnormally
        before or while processing the request.
...

(If I uncomment close_old_connections() in the database function, the test passes.)

OS and runtime environment: Linux, Postgres 10, Channels 2.1.6, Django 1.11.17, psycopg2 2.7.1.

This test is a bit of a hack -- sorry about that. But I trust the bug and solution are clear.

[andrewgodwin]

Agreed with the likely cause and resolution. I'll get this fix in today before I hang up maintainership soon.

[andrewgodwin]

Fixed in e2d6d65. Please verify that works for you and I'll stick it into a release.

[adamhooper]

@andrewgodwin Thanks! I can confirm that your change fixes this issue. @database_sync_to_async now reconnects expired connections prior to running the DB-dependent code.

[blueyed]

I wonder why channels has to close old connections here in the first place - wouldn't it be Django's job to use a new one? Why is this not happening / what is preventing it?
(there are no tests for this behavior)

[adamhooper]

@blueyed it is indeed Django's job to reap old connections. And it does reap old connections before and after every request handler is invoked.

@database_sync_to_async() lets Channels users access the database, skipping the regular request-handler routine. Therefore, Channels needs to mimic Django's behavior.

[blueyed]

I see, thanks!

[raceybe]

Sorry to resurrect this issue, but I think I'm seeing the same thing. I just recently switched from sqlite3 to a remote MariaDB installation and started having this issue. It follows whatever setting I configure on the MariaDB installation.

Django 2.26
Channels 2.3.0
MariaDB 10.4.?

I get (2006, 'MySQL server has gone away') after trying to establish a new channels connection with calls using database_sync_to_async(). I have set my Django DB CONN_MAX_AGE to 300 (5 minutes) and my MariaDB wait_timeout to 28800 (was 600 originally). All this did was extend the time before I saw the issue. Django calls to the DB work just fine well after the expiry time.

[blueyed]

@raceybe
Can you try #1290, please?

[raceybe]

@blueyed I'm open to things, I'm just not sure what "try" means...? Can I install a channel version based on that issue/pull request?

edit
Okay so I cloned your repository (https://github.com/blueyed/channels/tree/sync-db), modified setup.py for daphne dependencies (2.3 needs asgiref 3.0+) and after waiting for the connection to expire, I still get the same message.