Skip to content

Firing state change event immediately on property change#3451

Merged
YohDeadfall merged 7 commits intonpgsql:mainfrom
YohDeadfall:fixed-connection-state-event
Jan 14, 2021
Merged

Firing state change event immediately on property change#3451
YohDeadfall merged 7 commits intonpgsql:mainfrom
YohDeadfall:fixed-connection-state-event

Conversation

@YohDeadfall
Copy link
Contributor

Fixes #3442

@YohDeadfall YohDeadfall requested a review from roji as a code owner January 10, 2021 23:16
Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

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

Overall looks good! Let's see if @vonzshik has specific concerns in #3442.

//[Timeout(5000)]
public async Task BrokenLifecycle()
{
if (IsMultiplexing)
Copy link
Member

Choose a reason for hiding this comment

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

I think this test should pass with multiplexing too, no? You probably need to make sure the connection is bound before getting its PID, that can be done simply by starting a transaction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You made me to find another bug :(

Copy link
Contributor

@vonzshik vonzshik left a comment

Choose a reason for hiding this comment

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

Seems great to me.

@roji
Copy link
Member

roji commented Jan 12, 2021

@YohDeadfall looks like we all like what you're doing :) Take a look at my comments above.

@YohDeadfall YohDeadfall merged commit 65989b8 into npgsql:main Jan 14, 2021
@YohDeadfall YohDeadfall deleted the fixed-connection-state-event branch January 14, 2021 13:54
YohDeadfall added a commit that referenced this pull request Jan 28, 2021
@YohDeadfall
Copy link
Contributor Author

Backported to 5.0.4 via 8fe0cb0.

@roji
Copy link
Member

roji commented Jan 28, 2021

:(

I always backport immediately because I know I'll forget...

@vonzshik
Copy link
Contributor

@roji like #3485? 🤣

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

Successfully merging this pull request may close these issues.

StateChange event no longer fires when connection is observed to be killed

3 participants