Use signals to handle DB-close actions

[sevdog]

This is a proposal to solve #2118.

I have resurrected the signals which were used in 1.x (in the old worker component) and put them in the consumer and database_sync_to_async lifecycle.

I added a context-manager in consumer to handle this (yet I feel that it is not the best solution) to disconnect-connect them.

Since #2118 does not get triggered when sqlite is used as DB backend, it need a database which is accessed through a socket, I would like to discuss the best way to put a test against a "real" database in the CI.

[carltongibson]

Hey @sevdog

Since https://github.com/django/channels/issues/2118 does not get triggered when sqlite is used as DB backend, it need a database which is accessed through a socket, I would like to discuss the best way to put a test against a "real" database in the CI.

Postgres is pretty easy to set up in GHA:

jobs:
  build:
    runs-on: ubuntu-latest
    services:
      postgres:
        env:
          POSTGRES_USER: postgres
          POSTGRES_PASSWORD: postgres
          POSTGRES_DB: noumenal
        image: postgres:16.4
        ports: ["5432:5432"]
        options: --health-cmd pg_isready --health-interval 10s --health-timeout 5s --health-retries 5

Just initial take, I quite sceptical that this is the way forwards. It seems like a sledgehammer to crack a nut.

From your comments on the issue, I wondered if just patching at the start of the process, rather than trying to do it in such a narrow context as we are might be more robust.

It would be good to have some kind of semaphore which tells to the channels.db package something like "we are running tests, let the connection status to be handled by test runner".

Crudely, yes, let's try setting a flag and no-oping on that. Does that resolve the issue. (In which case we can think about being more sophisticated.)

cc @bigfootjon

[sevdog]

Thnk you @carltongibson, I understand your concern for the complexity of the implementation. Today I was in the mood of experimenting and I have overdone a bit with the whole context-manager/decorator part. Regarding the part of signals what I do not like in my current code is the part of using a signal for database_sync_to_async, which does not seem elegant to me. However I like the two signals for worker start/stop because I think it is a good option to allow users to hook in consumer lifecycle without having to do bad override of consumer's methods. I know that maybe a better naming can be found.

I have tested the current version of this branch with my project and I found that using django.test.TestCase it is useful to disconnect/reconnect signal handlers in setUpClass/tearDownClass. Once there was a TestCase in channels and I feel that it would be good to have it to reduce boiler-plate code and to optimize something.

Let me know what do you think and how do you feel about it. Cause this week I have a couple of days reserved to this task that I am willing to fulfill because I will feel better if I can update channels requirement in my project (which I cannot do as of now because tests fails in a non-predictable way due to #2118).

[carltongibson]

@sevdog I note this would require us to drop Django 4.2. That might be OK but I'm wondering if there isn't a smaller workaround you could apply.

Is there a reliable reproduce, in the test project from the issue maybe?

[sevdog]

@sevdog I note this would require us to drop Django 4.2. That might be OK but I'm wondering if there isn't a smaller workaround you could apply.

Did you mean something like:

# this may need to check that the AttributeError is raised from the signal object
try:
   await singl.asend(...)
except AttributeError:
   await sync_to_async(signal.send)(...)

Or something like:

import django
...

if django.VERSION >= (5, 0):
    await singl.asend(...)
else:
   await sync_to_async(signal.send)(...)

Is there a reliable reproduce, in the test project from the issue maybe?

I have already added this PR to the test matrix in https://github.com/sevdog/test-websocker-user-lock. I will update the tests after having finishing a change to simplify the handling of disconnect/connect using a mixin to be applied on TestCase. When this runs without any issue on the sample project I will try to add some model-related tests in channels.