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

feat: add disconnect() method to release resources deterministically #290

Merged
merged 2 commits into from
Mar 20, 2025

Conversation

SplotyCode
Copy link
Contributor

Introduces the close() method to ensure that all associated resources are released when closing a connection. This guarantees that no resources remain allocated, allowing garbage collection to reclaim memory safely. If no connection is open, the method is a noop. Future command dispatches will automatically establish a new connection if needed.

This is necessary to avoid potential resource leaks and to ensure deterministic behavior when closing a connection. In Beanstalk released jobs are immediately enqueued when the connection is closed. This means that even if we do not care about potential memory leaks, we still want to deterministic control when exactly the connection is closed.

@SamMousa
Copy link
Collaborator

I like the idea; when tests pass I'll merge this.

@SplotyCode SplotyCode changed the title feat: add close() method to release resources deterministically feat: add disconnect() method to release resources deterministically Mar 13, 2025
Introduces the disconnect() method to ensure that all associated resources
are released when closing a connection. This guarantees that no
resources remain allocated, allowing garbage collection to reclaim
memory safely. If no connection is open, the method is a noop.
Future command dispatches will automatically establish a new
connection if needed.

This is necessary to avoid potential resource leaks and to ensure
deterministic behavior when closing a connection. In Beanstalk  released jobs are immediately enqueued when the connection
is closed. This means that even if we do not care about potential
memory leaks, we still want to control when exactly the connection
is closed to avoid unintended side effects.
@SamMousa
Copy link
Collaborator

This is a break in BC, it means we need a major release. (Since we add a method to the interface, which other implementations don't have and therefore their code would break).

@SplotyCode
Copy link
Contributor Author

If we want to delay a mayor release I could also do something like #291 for v6 and we will add it to the interface later in v7.

@SamMousa
Copy link
Collaborator

I don't mind a major release; it's the correct thing. But need to make sure it's properly tagged, I think I can do that in the merge commit message.

@SamMousa
Copy link
Collaborator

On second thought: this disconnect should be on all interfaces. Even if you are just publishing you might want to disconnect etc.

@SamMousa
Copy link
Collaborator

@SplotyCode could you add the disconnect() method to all interfaces? Since it applies to implementations of each interface?

Moved the disconnect() method from PheanstalkManagerInterface to a new
PheanstalkClientInterface. The interfaces PheanstalkSubscriberInterface,
PheanstalkManagerInterface, and PheanstalkPublisherInterface now extend
PheanstalkClientInterface, ensuring a consistent API for all client-related
operations.

This refactoring improves interface segregation by grouping shared
functionality (and documentation) while keeping specific responsibilities separate.
@SamMousa
Copy link
Collaborator

This is OK! I will merge it, don't bother fixing the check it's not your fault.

@SplotyCode
Copy link
Contributor Author

Looks like phpunit/phpunit 12.0.9 is incompatible with phpstan/phpstan-phpunit 2.0.4. With phpunit/phpunit 12.0.8 it worked. From my understanding the ci error is unrelated to the changes in this pr

@SplotyCode
Copy link
Contributor Author

@SamMousa SamMousa merged commit 1b85ec2 into pheanstalk:master Mar 20, 2025
3 of 4 checks passed
Copy link

🎉 This PR is included in version 7.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@SplotyCode
Copy link
Contributor Author

Ah, I didn't see your comment. Thanks for the heads-up! We can temporarily force a lower PHPUnit version with
composer require "phpunit/phpunit:>=12.0.0 <12.0.9"
while Sebastian and the team sort out the incompatibilities.
Thanks for the release management!

@SamMousa
Copy link
Collaborator

No need, the extension causing the issue is no longer needed for this project!
Thanks for the contribution(s)!

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

Successfully merging this pull request may close these issues.

2 participants