FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Postgres backend: handle connection loss by asafc · Pull Request #66 · encode/broadcaster · GitHub

This repository was archived by the owner on Aug 19, 2025. It is now read-only.
/ broadcaster Public archive

Postgres backend: handle connection loss - #66

Open
asafc wants to merge 1 commit into
encode:masterfrom
permitio:bugfix/handle_postgres_disconnection
ConversationCommits (1)ChecksFiles changed
Open

Postgres backend: handle connection loss#66
asafc wants to merge 1 commit into
encode:masterfrom
permitio:bugfix/handle_postgres_disconnection

Conversation

asafc commented Jun 12, 2022

Copy link
Copy Markdown

This bugfix handle a situation where broadcaster does not detect the Postgres backend terminating the connection.

A user of Broadcaster will not be able to handle disconnects in their code.

i.e: the next loop will not exit:

async def chatroom_ws_sender(websocket):
    async with broadcast.subscribe(channel="chatroom") as subscriber:
        async for event in subscriber:
            await websocket.send_text(event.message)

when either:

  1. the Postgres db is down
  2. Postgres terminated an idle session (long lived session) due to either idle_session_timeout or idle_in_transaction_session_timeout (see here)

dariosky commented Dec 21, 2022

Copy link
Copy Markdown

Hi @asafc - this was a good start.
It is missing the handling of the disconnection though. I ended up playing with this for a bit and change both the base and the postgres backend to reconnect and resubscribe to the channels so it works for my use-case.
Let me know if you want me to add to this merge-request.

roekatz commented Mar 26, 2023

Copy link
Copy Markdown

Hi @dariosky,
I was involved in this fix on our fork https://github.com/permitio/broadcaster.

I don't believe automatically reconnecting is a good idea: when connection is loss messages could be missed,
therefor the caller should be notified - any initialization required to catch up with potentially missing updates.

On our use case - disconnecting (marked by ending the iterator) is more than enough (then the caller can reconnect).
It's also possible to have some on_reconnect callback, so caller could be notified while connection is re-established - but IMO it doesn't really add a lot of value :)

WDYT?

dariosky commented Mar 27, 2023

Copy link
Copy Markdown

Having an on_reconnect parameter is a good idea - so the caller can handle the eventual gap-filling...
In my case I didn't need it - I had just a reconnect on-error loop and was keeping the memory of the subscriptions to restore them when the reconnection happened...

roekatz commented Mar 27, 2023

Copy link
Copy Markdown

Not sure I understand - have you pushed that fix? (or plan on pushing it?)

dariosky commented Mar 27, 2023

Copy link
Copy Markdown

I didn't (I asked above if it can be useful) - let me add it here :)

dariosky commented Mar 27, 2023

Copy link
Copy Markdown

See here: permitio#4

alex-oleshkevich mentioned this pull request Apr 3, 2024
Closed
19 tasks
logankaser mentioned this pull request Jun 12, 2024
Closed
logankaser reviewed Jun 12, 2024
View reviewed changes
Comment thread broadcaster/_base.py
await queue.put(event)

# Ubsubscribe all
for queue in sum([list(qs) for qs in self._subscribers.values()], []):

logankaser Jun 12, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality 
# Unsubscribe all
for queue_set in self._subscribers.values():
    for queue in queue_set:
        await queue.put(None)

Seems like a lot less wasteful / clear what's going on.
As far as I know, you can loop over sets directly in python, no need
to convert to a list.

alex-oleshkevich commented Jun 27, 2024

Copy link
Copy Markdown
Contributor

This requires extra testing. Will not include in 0.3

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
logankaser logankaser left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL