Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"Bad state: StreamSink is closed" - for version 3.4.3. #389

Open
SandPod opened this issue Nov 17, 2024 · 3 comments
Open

"Bad state: StreamSink is closed" - for version 3.4.3. #389

SandPod opened this issue Nov 17, 2024 · 3 comments

Comments

@SandPod
Copy link

SandPod commented Nov 17, 2024

Unfortunately, I can't provide a detailed report on the issue this time.

Description

In Serverpod we use this driver as our postgres driver and got an issue reported from a user that version 3.4.3 would consistently disconnect from the database with the error Bad state: StreamSink is closed after running the .

Bug report here with stack trace here: serverpod/serverpod#2965

A temporary fix for the user was to simply pin the version to 3.4.0 where this behavior was not experienced.

I have asked the user if this was also an issue with versions 3.4.2 and 3.4.1.

Questions

  1. Have any lifecycle changes been introduced (intentional or unintentional) in the new versions that might need to be handled in the framework to preserve the database connection?
  2. I noticed this change: Fix: prevent hanging state by forwarding protocol-level parsing errors into the message stream. #388, if a parsing error happens on one connection for a connection pool, would that close the connection pool?
  3. Any additional information that might be required to understand why this issue is experienced?
@isoos
Copy link
Owner

isoos commented Nov 17, 2024

A temporary fix for the user was to simply pin the version to 3.4.0 where this behavior was not experienced.

That's probably a bad idea, as the last three release was about issues that made the transaction or the connection hanging, without notifying the user, or reporting it the wrong place. I think all of these fixes are better for the users than going back to a state where they are not exposed.

Have any lifecycle changes been introduced (intentional or unintentional) in the new versions that might need to be handled in the framework to preserve the database connection?

I think connection pools are the preferred way to handle this. I use a relatively low timeout (1 hour) after which I renew the connections.

if a parsing error happens on one connection for a connection pool, would that close the connection pool?

It should close the connection, but not the pool. There may be unexpected issues if the system also has pgbouncer or other intermediate connection controller in-between.

Any additional information that might be required to understand why this issue is experienced.

We should have a repeatable test case, otherwise these are hard to debug or reason about. Also, components and their versions that are being used.

@SandPod
Copy link
Author

SandPod commented Nov 17, 2024

That's probably a bad idea, as the last three release was about issues that made the transaction or the connection hanging, without notifying the user, or reporting it the wrong place.

I agree with this.

According to the user report, all connection attempts to the database after the stream sink was closed failed with the new version. In the framework, we create a connection pool when the server is started and then use a new connection for each request.

Since you mention "without notifying the user or reporting it in the wrong place," given the recent changes, is there any chance that an exception that was previously silenced could bubble up and render the connection pool in an invalid state?

We should have a repeatable test case, otherwise these are hard to debug or reason about. Also, components and their versions that are being used.

We will work on trying to identify a solid repro for this.

@isoos
Copy link
Owner

isoos commented Nov 17, 2024

Since you mention "without notifying the user or reporting it in the wrong place," given the recent changes, is there any chance that an exception that was previously silenced could bubble up and render the connection pool in an invalid state?

I don't think so, because the pool logic is simple and it should discard connections that are not in a good state. However, bugs and edge cases may still happen.

Thanks for looking into it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants