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

Fixes for scan/sscan/zscan/hscan & transaction/multi handling #181

Merged
merged 16 commits into from
Apr 18, 2023

Conversation

Xon
Copy link

@Xon Xon commented Apr 10, 2023

  • Fixes type hinting on for scan/sscan/zscan/hscan
  • Aligns behavior of scan/sscan/zscan/hscan with phpredis of how an empty/falsy iterator is handled.
  • Fix errors in exec() could cause the redis connection and credis internal state to become out of sync
  • Prevent scan/sscan/zscan/hscan from being used in pipeline/multi mode, as phpredis would cause trigger a fatal php error instead of throwing an exception in this case
  • Fix calling mget([]) in standalone mode didn't match phpredis mode
  • Fix multi mode failing when commands where issued between multi() and pipeline(). Recommended usage is however pipeline()-> multi()->...->exec()

This breaks pipeline mode for these functions as they return early, but mget was already doing this. Documented in #182

@colinmollenhour
Copy link
Owner

@Xon Thanks for the PR. I'm not sure about it though since it would break code that uses chaining, or is it just never practical to use these methods with chaining?

@Xon
Copy link
Author

Xon commented Apr 16, 2023

Using this particular methods in chaining mode is fairly difficult as you need to returned iterator token for the next call, but you could batch them with other methods I guess.

I was planning on adding a PR after this to patch these methods so pipeline mode could push an early result into the command array so the methods are never sent to redis. This would fix the compatibility issues with pipelining

@Xon Xon force-pushed the scan-fix branch 2 times, most recently from ced0b48 to 5adc19e Compare April 17, 2023 04:45
@Xon Xon marked this pull request as draft April 17, 2023 06:06
@Xon
Copy link
Author

Xon commented Apr 17, 2023

Redis::scan(): Can't call SCAN commands in multi or pipeline mode!

So it looks like phpredis just doesn't allow scan in pipeline/multi mode, and a mget([]) is silently dropped from the exec() results.

I'll add some test to catch these

@Xon
Copy link
Author

Xon commented Apr 17, 2023

After a bit more testing, using the scan-related commands never worked in pipeline/multi mode with standalone credis mode. The iterator was never updated as expected

@Xon Xon marked this pull request as ready for review April 17, 2023 11:22
@Xon
Copy link
Author

Xon commented Apr 17, 2023

Forcing the connection closed and resetting pipelining/transaction state ended up being required to actually debug why unrelated tests where failing. redis/credis getting out of sync on the protocol level is yuck :(

This ended up being a lot more involve but fixes #182 so the same behavior is observed between phpredis and standalone mode when in transaction/pipeline mode. In previous cases calling these functions in standalone mode would either error or be unusable.

I've updated the PR with a description of the various bugs fixed. Most of them are actually relatively obscure that wouldn't be hit in normal usage. Basically pipeline()->multi() should be the strongly recommended pattern when interacting with the multi() command.

@Xon Xon changed the title Fixes for scan/sscan/zscan/hscan Fixes for scan/sscan/zscan/hscan & transaction/multi handling Apr 17, 2023
@colinmollenhour
Copy link
Owner

Nice work @Xon!

@colinmollenhour colinmollenhour merged commit 2881043 into colinmollenhour:master Apr 18, 2023
4 checks passed
@Xon Xon deleted the scan-fix branch April 18, 2023 16:52
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.

None yet

2 participants