-
Notifications
You must be signed in to change notification settings - Fork 4
PubSub implementation #62
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
base: main
Are you sure you want to change the base?
Conversation
- Support subscribe, unsubscribe, psubscribe, and punsubscribe in standalone and cluster clients. Partition callback lookup by client type. - Allow multiple clients with their own callbacks. Each client has one callback that can be overwritten with another subscribe call. Unsubscribing is not required. - Cleanup callbacks on subscribe. Use atomic-reference counting and validation flags to avoid race conditions where a callback is invoked during destruction, unsubscription, or callback overwriting without blocking. Signed-off-by: James Duong <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request implements PubSub (Publish/Subscribe) functionality for Valkey GLIDE, supporting both standalone and cluster clients with safe callback management and reference counting to prevent race conditions during callback lifecycle operations.
Key changes:
- Added
subscribe
,unsubscribe
,psubscribe
, andpunsubscribe
commands with callback support - Implemented separate callback registries for standalone and cluster clients to prevent cross-client callback invocation
- Introduced atomic reference counting with validation flags for thread-safe callback management during destruction and overwriting
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 12 comments.
Show a summary per file
File | Description |
---|---|
valkey_glide_pubsub.h | Header file defining PubSub structures, callback wrappers, and method macros |
valkey_glide_pubsub.c | Core PubSub implementation with callback management and command execution |
valkey_glide_core_commands.c | Updated client creation to register PubSub callbacks and client mappings |
valkey_glide_commands_common.h | Modified function signatures to include valkey_glide_object parameter |
valkey_glide_cluster.stub.php | Uncommented PubSub method declarations for cluster client |
valkey_glide_cluster.c | Implemented PubSub methods using macros for cluster client |
valkey_glide.stub.php | Added comprehensive PubSub method documentation and signatures |
valkey_glide.c | Implemented PubSub methods and callback cleanup in destructor |
tests/ValkeyGlideTest.php | Added extensive test coverage for PubSub functionality |
tests/ValkeyGlideBatchTest.php | Added test to verify PubSub commands are blocked in batch mode |
config.m4 | Added valkey_glide_pubsub.c to build configuration |
common.h | Added pubsub_callback and is_cluster_client fields to valkey_glide_object |
README.md | Added PubSub usage documentation and examples |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
* | ||
* For multiple independent callbacks, use separate client instances. | ||
* | ||
* @param array|string $channels Channel name(s) to subscribe to |
Copilot
AI
Oct 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation indicates the parameter accepts array|string
, but the C implementation (valkey_glide_pubsub.c line 339) only accepts arrays with Z_PARAM_ARRAY. This inconsistency between documentation and implementation will cause runtime errors when users pass strings.
Copilot uses AI. Check for mistakes.
* | ||
* For multiple independent callbacks, use separate client instances. | ||
* | ||
* @param array|string $patterns Pattern(s) to subscribe to |
Copilot
AI
Oct 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation indicates the parameter accepts array|string
, but the C implementation (valkey_glide_pubsub.c line 374) only accepts arrays with Z_PARAM_ARRAY. This inconsistency between documentation and implementation will cause runtime errors when users pass strings.
Copilot uses AI. Check for mistakes.
valkey_glide.stub.php
Outdated
/** | ||
* Unsubscribe from channels | ||
* | ||
* @param array|string $channels Channel name(s) to unsubscribe from |
Copilot
AI
Oct 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation indicates the parameter accepts array|string
, but the C implementation (valkey_glide_pubsub.c line 410) only accepts arrays or null with Z_PARAM_ARRAY_OR_NULL. This inconsistency between documentation and implementation will cause runtime errors when users pass strings.
Copilot uses AI. Check for mistakes.
valkey_glide.stub.php
Outdated
* For multiple independent callbacks, use separate client instances. | ||
* | ||
* @param array|string $channels Channel name(s) to subscribe to | ||
* @param callable $callback Callback function to handle messages: function($redis, $channel, $message) |
Copilot
AI
Oct 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The callback signature shows 3 parameters, but according to the test at line 7582 in ValkeyGlideTest.php, the actual callback receives the redis instance as the first parameter. The documentation should clarify that $redis is the ValkeyGlide client instance, not a generic 'redis' reference.
Copilot uses AI. Check for mistakes.
valkey_glide.stub.php
Outdated
* For multiple independent callbacks, use separate client instances. | ||
* | ||
* @param array|string $patterns Pattern(s) to subscribe to | ||
* @param callable $callback Callback function to handle messages: function($redis, $pattern, $channel, $message) |
Copilot
AI
Oct 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The callback signature documentation conflicts with the test at line 7675 and actual implementation at line 142-143 in valkey_glide_pubsub.c. The actual callback receives parameters in order: $redis, $channel, $message, $pattern (with pattern as 4th arg), not $redis, $pattern, $channel, $message as documented.
* @param callable $callback Callback function to handle messages: function($redis, $pattern, $channel, $message) | |
* @param callable $callback Callback function to handle messages: function($redis, $channel, $message, $pattern) |
Copilot uses AI. Check for mistakes.
Signed-off-by: James Duong <[email protected]>
Co-authored-by: Copilot <[email protected]> Signed-off-by: James Duong <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 9 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Signed-off-by: James Duong <[email protected]>
Signed-off-by: James Duong <[email protected]>
Signed-off-by: James Duong <[email protected]>
Signed-off-by: James Duong <[email protected]>
Signed-off-by: James Duong <[email protected]>
Signed-off-by: James Duong <[email protected]>
Signed-off-by: James Duong <[email protected]>
Not implemented in the core Signed-off-by: James Duong <[email protected]>
Signed-off-by: James Duong <[email protected]>
Signed-off-by: James Duong <[email protected]>
Signed-off-by: James Duong <[email protected]>
Issue link
This Pull Request is linked to issue (URL): [REPLACE ME]
Checklist
Before submitting the PR make sure the following are checked: