Skip to content

Conversation

@Marsup
Copy link
Contributor

@Marsup Marsup commented Jul 2, 2025

Why

As explained in #2619, the resolution order of setTimeout and bzpopmin under certain conditions is wrong.

How

To reestablish the correct order, I'm scheduling the disconnection for the next tick to let bzpopmin eventually resolve properly.

Additional Notes (Optional)

I'm honestly not sure how to add non-regression testing on this, I'd like some advice on this if possible.

I also removed the async modifier of the timeout callback, not really sure why it was there.

@Marsup Marsup force-pushed the fix/bzpopmin-race-condition branch from 61aaf12 to f7ce6b6 Compare July 3, 2025 22:30
Copy link
Contributor

@manast manast left a comment

Choose a reason for hiding this comment

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

This feels like hitting the can down the road instead of actually resolving the possible underlying issue, although I am not convinced yet there is any issue :/

@Marsup
Copy link
Contributor Author

Marsup commented Jul 7, 2025

I don't like that solution myself either, but I know it works, and it'll probably keep working for the foreseeable future. The only alternative would be for ioredis to go into the microtask queue to go 1st, but I haven't been able to figure out in the driver why it was queuing itself after the other timers.

@manast
Copy link
Contributor

manast commented Jul 7, 2025

I don't like that solution myself either, but I know it works, and it'll probably keep working for the foreseeable future. The only alternative would be for ioredis to go into the microtask queue to go 1st, but I haven't been able to figure out in the driver why it was queuing itself after the other timers.

It is solving your particular symptom, however as it is not fully clear what is the issue it is not really solving it.

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.

2 participants