- Add isPubSubMessage() function to identify pub/sub message types
- Filter out pub/sub messages in ProcessPendingNotifications
- Allow pub/sub system to handle its own messages without interference
- Process only cluster/system push notifications (MOVING, MIGRATING, etc.)
- Add comprehensive test coverage for filtering logic
Pub/sub message types filtered:
- message (regular pub/sub)
- pmessage (pattern pub/sub)
- subscribe/unsubscribe (subscription management)
- psubscribe/punsubscribe (pattern subscription management)
- smessage (sharded pub/sub, Redis 7.0+)
Benefits:
- Clear separation of concerns between pub/sub and push notifications
- Prevents interference between the two messaging systems
- Ensures pub/sub messages reach their intended handlers
- Eliminates message loss due to incorrect interception
- Improved system reliability and performance
- Better resource utilization and message flow
Implementation:
- Efficient O(1) switch statement for message type lookup
- Case-sensitive matching for precise filtering
- Early return to skip unnecessary processing
- Maintains processing of other notifications in same batch
- Applied to all processing points (WithReader, Pool.Put, isHealthyConn)
Test coverage:
- TestIsPubSubMessage - Function correctness and edge cases
- TestPubSubFiltering - End-to-end integration testing
- Mixed message scenarios and handler verification
- Add 100% test coverage for Registry (NewRegistry, RegisterHandler, UnregisterHandler, GetHandler, GetRegisteredPushNotificationNames)
- Add 100% test coverage for Processor (NewProcessor, GetHandler, RegisterHandler, UnregisterHandler)
- Add 100% test coverage for VoidProcessor (NewVoidProcessor, GetHandler, RegisterHandler, UnregisterHandler, ProcessPendingNotifications)
- Add comprehensive tests for ProcessPendingNotifications with mock reader testing all code paths
- Add missing UnregisterHandler method to VoidProcessor
- Remove HandleNotification method reference from RegistryInterface
- Create TestHandler, MockReader, and test helper functions for comprehensive testing
Test coverage achieved:
- Registry: 100% coverage on all methods
- VoidProcessor: 100% coverage on all methods
- Processor: 100% coverage except ProcessPendingNotifications (complex RESP3 parsing)
- Overall package coverage: 71.7% (limited by complex protocol parsing logic)
Test scenarios covered:
- All constructor functions and basic operations
- Handler registration with duplicate detection
- Protected handler unregistration prevention
- Empty and invalid notification handling
- Error handling for all edge cases
- Mock reader testing for push notification processing logic
- Real proto.Reader testing for basic scenarios
Benefits:
- Comprehensive test coverage for all public APIs
- Edge case testing for error conditions
- Mock-based testing for complex protocol logic
- Regression prevention for core functionality
- Documentation through test examples
- Remove unnecessary handlerWrapper complexity from push notifications
- Use separate maps for handlers and protection status in registry
- Store handlers directly without indirection layer
- Maintain same instance identity for registered/retrieved handlers
- Preserve all protected handler functionality with cleaner implementation
Changes:
- internal/pushnotif/registry.go: Use separate handlers and protected maps
- push_notifications.go: Remove handlerWrapper, store handlers directly
- Maintain thread-safe operations with simplified code structure
Benefits:
- Reduced memory overhead (no wrapper objects)
- Direct handler storage without type conversion
- Cleaner, more maintainable code
- Same functionality with better performance
- Eliminated unnecessary complexity layer
- Preserved all existing behavior and safety guarantees
- Remove enabled field from PushNotificationProcessor struct
- Remove IsEnabled() and SetEnabled() methods from processor interface
- Remove enabled parameter from NewPushNotificationProcessor()
- Update all interfaces in pool package to remove IsEnabled requirement
- Simplify processor logic - if processor exists, it works
- VoidPushNotificationProcessor handles disabled case by discarding notifications
- Update all tests to use simplified interface without enable/disable logic
Benefits:
- Simpler, cleaner interface with less complexity
- No unnecessary state management for enabled/disabled
- VoidPushNotificationProcessor pattern handles disabled case elegantly
- Reduced cognitive overhead - processors just work when set
- Eliminates redundant enabled checks throughout codebase
- More predictable behavior - set processor = it works
- Add PushNotificationProcessor field to pool.Conn for connection-level processing
- Modify connection pool Put() and isHealthyConn() to handle push notifications
- Process pending push notifications before discarding connections
- Pass push notification processor to connections during creation
- Update connection pool options to include push notification processor
- Add comprehensive test for connection health check integration
This prevents connections with buffered push notification data from being
incorrectly discarded by the connection health check, ensuring push
notifications are properly processed and connections are reused.
- At present, the `scan` command is dispatched to a random slot.
- As far as I can tell, the scanX family of commands are not cluster aware (e.g. don't redirect the client to the correct slot).
- You can see [here](869dc0bb66/src/main/java/redis/clients/jedis/ShardedCommandObjects.java (L101)), the Jedis client calling `processKey` on the match argument, and this is what this PR also does.
We've had this patch running in production, and it seems to work well for us.
For further thought:
- Continuing looking at other Redis clients (e.g. Jedis), they outright [reject as invalid](869dc0bb66/src/main/java/redis/clients/jedis/ShardedCommandObjects.java (L98)) any scan command that does not include a hash-tag. Presumably this has the advantage of users not being surprised when their scan produces no results when a random server is picked.
- Perhaps it would be sensible for go-redis to do the same also?
Co-authored-by: Nedyalko Dyakov <1547186+ndyakov@users.noreply.github.com>
* wip
* update documentation
* add streamingcredentialsprovider in options
* fix: put back option in pool creation
* add package level comment
* Initial re authentication implementation
Introduces the StreamingCredentialsProvider as the CredentialsProvider
with the highest priority.
TODO: needs to be tested
* Change function type name
Change CancelProviderFunc to UnsubscribeFunc
* add tests
* fix race in tests
* fix example tests
* wip, hooks refactor
* fix build
* update README.md
* update wordlist
* update README.md
* refactor(auth): early returns in cred listener
* fix(doctest): simulate some delay
* feat(conn): add close hook on conn
* fix(tests): simulate start/stop in mock credentials provider
* fix(auth): don't double close the conn
* docs(README): mark streaming credentials provider as experimental
* fix(auth): streamline auth err proccess
* fix(auth): check err on close conn
* chore(entraid): use the repo under redis org
* chore: optimize function `ReplaceSpaces`
Signed-off-by: fukua95 <fukua95@gmail.com>
* trigger CI again because the bug of docker
Signed-off-by: fukua95 <fukua95@gmail.com>
* trigger CI again because the bug of docker
Signed-off-by: fukua95 <fukua95@gmail.com>
---------
Signed-off-by: fukua95 <fukua95@gmail.com>
* chore: extract benchmark tests
* wip
* enable pubsub tests
* enable ring tests
* stop tests with build redis from source
* start all tests
* mix of makefile and action
* add sentinel configs
* fix example test
* stop debug on re
* wip
* enable gears for redis 7.2
* wip
* enable sentinel, they are expected to fail
* fix: linter configuration
* chore: update re versions
* return older redis enterprise version
* add basic codeql
* wip: increase timeout, focus only sentinel tests
* sentinels with docker network host
* enable all tests
* fix flanky test
* enable example tests
* tidy docker compose
* add debug output
* stop shutingdown masters
* don't test sentinel for re
* skip unsuported addscores
* Update README
bump go version in CI
* Update README.md
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* Update CONTRIBUTING.md
add information about new test setup
---------
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* Added the ability to set a connection growth limit when there are not enough connections in the pool using MaxActiveConns
* fix comment
* fix
* fix
---------
Co-authored-by: ofekshenawa <104765379+ofekshenawa@users.noreply.github.com>
* run go fix ./...
Signed-off-by: Tiago Peczenyj <tpeczenyj@weborama.com>
* run make fmt
Signed-off-by: Tiago Peczenyj <tpeczenyj@weborama.com>
* fix go vet ./... issues
* Update README.md
Reorder imports with the rules defined in the Makefile
as if we run `make fmt`
* run gofumpt -w .
* update Makefile to use gofumpt instead gofmt
* increment makefile
* format test
* format tests
Signed-off-by: Tiago Peczenyj <tpeczenyj@weborama.com>
---------
Signed-off-by: Tiago Peczenyj <tpeczenyj@weborama.com>
Co-authored-by: ofekshenawa <104765379+ofekshenawa@users.noreply.github.com>
