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

add support for connection closed listeners #525

Merged
merged 3 commits into from
Jul 18, 2020

Conversation

ioistired
Copy link
Contributor

#421 (comment)

  • I'm not sure that the API is what I want. It's called a "cleanup listener" internally and a "close listener" externally.
  • I don't know how to add tests for this.

Other than that, how does this look? The following code demonstrates the functionality. It prints True after a postgres server restart, then exits:

#!/usr/bin/env python3

import asyncpg
import asyncio

async def amain():
	conn = await asyncpg.connect()
	cleanup_ran = False
	def close_listener(_):
		nonlocal cleanup_ran
		cleanup_ran = True
	conn.add_close_listener(close_listener)
	while await asyncio.sleep(1, True):
		if conn.is_closed():
			print(cleanup_ran)
			break

if __name__ == '__main__':
	asyncio.run(amain())

@elprans
Copy link
Member

elprans commented Jan 13, 2020

I'm not sure that the API is what I want. It's called a "cleanup listener" internally and a "close listener" externally.

close_listener is better.

I don't know how to add tests for this.

Look at test_adversity.py for an example. There is a special proxy to simulate connection issues. You would need to add a method to drop the connection to it (_testbase/fuzzer.py)

@ioistired
Copy link
Contributor Author

Please clarify.

  • Which class needs a "drop connection" method? The TCPFuzzingProxy class already has a trigger_connectivity_loss() method. What should mine do differently?
  • Should my test cases be in test_adversity.py as well? If so, should they be in a separate class that also extends tb.ProxiedClusterTestCase, or should they be added as methods to TestConnectionLoss?

@ioistired ioistired marked this pull request as ready for review January 23, 2020 02:52
@ioistired
Copy link
Contributor Author

I am un-drafting this PR because I don't know how to write proper tests for this code. Maintainers are free to commit to my PR, and if anyone else wants to pick up the PR, feel free to make a new one based on mine.

@ioistired
Copy link
Contributor Author

Can any maintainer please take a look at this and try to write up some test cases?

@stalkerg
Copy link

it's a very important feature, can somebody check it?

Termination listener reads better.
Copy link
Contributor Author

@ioistired ioistired left a comment

Choose a reason for hiding this comment

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

Can you elaborate on the name "termination listener"? How does "close listener" read worse?

@elprans
Copy link
Member

elprans commented May 4, 2020

In the phrase "close listener" the word "close" reads like a distance adjective, i.e. "a near listener". "Termination listener" is simply better English.

@elprans elprans merged commit 8141b93 into MagicStack:master Jul 18, 2020
@ioistired ioistired deleted the on_close branch July 18, 2020 22:43
@ioistired
Copy link
Contributor Author

Thank you so much!

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.

3 participants