support RW ready in waiting functions

[dlax]

Replaces #107 and would be needed for the pipeline mode #93.

[dvarrazzo]

Out of curiosity, have you run any benchmark on your async loop changed? I assume it would be even a tad more performing than before...

[dlax]

Out of curiosity, have you run any benchmark on your async loop changed? I assume it would be even a tad more performing than before...

No, I haven't.

But in fact, while working on anyio support #146 I think we'll need to get back to the Event approach (and no longer use Future objects). Now, as timed passed, I think that handling Ready.RW is also achievable with the Event approach. So, maybe it's worth reverting to the previous approach? What do you think? (I fine with either options.)

[dvarrazzo]

Let's recap:

• in order to implement the pipeline we need to handle Ready.RW. No RW no pipeline.
• you have an implementation ready (Futures-based)
• There could be an anyio implementation (does it work already with the pipelines? I haven't read that code yet)
• There could be an Event-based implementation too (but not written yet)

Is this right?

Afaics this choice is important but limited in scope: it only affects the wait loop and we can replace implementation any moment. I think we can proceed with the Futures so we have the interface set up and then we can benchmark to figure out the implementation we prefer.

Also, the Event implementation is the first I wrote and you might have seen from the comments that I've never been entirely sold on it: happy to know if there's a better way.

[dlax]

There could be an Event-based implementation too (but not written yet)

Actually, I have written that one too, see dlax@e0bea08

Also, the Event implementation is the first I wrote and you might have seen from the comments that I've never been entirely sold on it: happy to know if there's a better way.

Yes, seen that. On the other hand, I then saw that anyio's implementation was also using Event object (see https://github.com/agronholm/anyio/blob/3.3.4/src/anyio/_backends/_asyncio.py#L1564); so perhaps it's not so bad?

[dlax]

There could be an anyio implementation (does it work already with the pipelines? I haven't read that code yet)

The proposed anyio implementation does not support RW-ready at it is based on current master (which does not support this), but I can easily adjust it after this change (either Future- or Event-based). Not a big deal.

[dlax]

So my point was that, if there are chances that the anyio proposal gets accepted at some point, then there is no real advantage in picking the Future-based implementation and benchmarking it since we'd eventually need to move back to an Event-based approach.

[dlax]

Suggested changes applied in the last push:

diff --git a/psycopg/psycopg/waiting.py b/psycopg/psycopg/waiting.py
index eeafe519..7bdf9f5c 100644
--- a/psycopg/psycopg/waiting.py
+++ b/psycopg/psycopg/waiting.py
@@ -219,7 +219,7 @@ def wait_epoll(
             ev = fileevs[0][1]
             ready = 0
             if ev & ~select.EPOLLOUT:
-                ready |= Ready.R
+                ready = Ready.R
             if ev & ~select.EPOLLIN:
                 ready |= Ready.W
             assert s & ready
diff --git a/tests/test_waiting.py b/tests/test_waiting.py
index 092ab42d..10e99bbb 100644
--- a/tests/test_waiting.py
+++ b/tests/test_waiting.py
@@ -22,7 +22,7 @@ timeouts = [
 ]
 
 
-linux = pytest.mark.skipif(
+skip_if_not_linux = pytest.mark.skipif(
     not sys.platform.startswith("linux"), reason="non-Linux platform"
 )
 
@@ -62,7 +62,7 @@ waits, wids = list(zip(*waits_and_ids))
 
 @pytest.mark.parametrize("waitfn", waits, ids=wids)
 @pytest.mark.parametrize("wait, ready", zip(waiting.Wait, waiting.Ready))
-@linux
+@skip_if_not_linux
 def test_wait_ready(waitfn, wait, ready):
     def gen():
         r = yield wait
@@ -131,7 +131,7 @@ async def test_wait_async(pgconn):
 
 @pytest.mark.asyncio
 @pytest.mark.parametrize("wait, ready", zip(waiting.Wait, waiting.Ready))
-@linux
+@skip_if_not_linux
 async def test_wait_ready_async(wait, ready):
     def gen():
         r = yield wait

pipeline2 branch is out of sync from this one, now; I can rebase it as well or wait until we decide.

[dvarrazzo]

There could be an Event-based implementation too (but not written yet)

Actually, I have written that one too, see dlax@e0bea08

Good: this implementation seems minimally invasive. For me we can start from this, so we can reason about the higher level pipeline feature, and then we can go back on the choice of whether to leave the Event approach, or to move to Future or to anyio.

[dlax]

Ok; I've pushed this implementation here.
Also added c017623 to always remove reader/writer even in case of wait failure.