Implement notification_stream for AsyncPgConnection #251
Conversation
There was a problem hiding this comment.
Thanks for opening this PR. That looks already like a really good starting point. I've hopefully added comments to all your questions.
Before merging I also would like to see at least this test from diesel being ported to verify that the function works as expected.
|
@weiznich I updated the impl in the right direction, I only have one more major question I'll add as a comment. Tests and doc still missing |
src/pg/mod.rs
Outdated
|
|
||
| pub fn notification_stream( | ||
| &mut self, | ||
| ) -> impl futures_core::Stream<Item = diesel::pg::PgNotification> + '_ { |
There was a problem hiding this comment.
It's about this signature. I changed it to Stream to match the channel, but now I think maybe it's wrong because you said it should be Stream<QueryResult>.
That said, the poll_messages function exposes Ok(Notification | Notice) | Err, so if I get an error I can't be sure it's from a notification, a notice, or anything. It's basically just an error.
So it feels to me that I should either output the 3 possible cases, or just pick the one I want (notification). But I'm not sure of this. Anyway, I think diesel sync might have gone through the same question?
There was a problem hiding this comment.
I think it needs to be Stream<QueryResult> as we currently miss one important point: We still need to poll the drive_connection future as well as part of the stream, as otherwise we won't get any new notifications while the user polls on the channel we returned. I think that connection future might return an error (via the error channel), that then must be returned from the notification stream.
(Hopefully that's "clear" and not expressed in a too confusing way)
There was a problem hiding this comment.
Yeah, that actually makes sense. I implemented it on 297c383.
One leftover question, does it make sense to have the error go on both error_tx and notification_tx? That's what I ended up leaving with but I was not really sure.
|
@weiznich I think current implementation might be good enough. If we're getting closer, I'll work on the test next |
|
Added documentation and doctest based on sync diesel version. 685f0ba I think this is ready for review now |
Goal
This PR aims to implement a way for an AsyncPgConnection user to receive notifications from postgresql.
Implementation
It leverages the already present functionality of the underlying connection. So this PR merely wants to expose the messages.
Limitations
As @weiznich mentioned on the discussion, this will only work if the connection is present (and it might not be, depending on other constructors).
The easier fix is obviously not supporting the alternative constructors, but since we are splitting functionality it's always a good place to question if they are needed.
This PR is a DRAFT aimed to ask questions to mantainers