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

Update hiredis-cluster to 0.14.0 from 0.11.0 #48

Merged
merged 9 commits into from
Sep 21, 2024

Conversation

plainbanana
Copy link
Owner

@plainbanana plainbanana commented Sep 6, 2024

fix: #45

This commit updates the Minilla version from 3.1.22 to 3.1.23 in the META.json file. This change ensures that the metadata reflects the correct versioning.
To clear HIRCLUSTER_FLAG_SHUTDOWN flag, explicitly call redisClusterConnect2 immediately after fork.
@plainbanana
Copy link
Owner Author

plainbanana commented Sep 20, 2024

Need for Fork Behavior Change and Considerations

To ensure fork-safe code and reuse the cluster topology obtained by the parent process in the child process, several adjustments are made at Redis::Cluster::Fast using hiredis-cluster 0.11.0 .

Previous Behavior (0.10.0)

  • Topology updates were performed synchronously when commands were issued.
  • Right after fork, the event_base would have no unissued handlers, only holding the connection.
    • Issuing event_reinit followed by redisClusterAsyncDisconnect was sufficient to handle this.

Previous Behavior (0.11.0)

  • Topology updates were performed asynchronously.
  • Right after fork, the event_base may have unissued handlers (e.g., CLUSTER NODES).
  • [BUG] Event loop occasionally hangs after redisClusterAsyncDisconnect under high connection error conditions

Upcoming Behavior (0.14.0)

  • Topology updates will be performed asynchronously.
  • Right after fork, the event_base may have unissued handlers (e.g., CLUSTER NODES).
  • redisClusterAsyncDisconnect related bug is fixed 🎉
    • When redisClusterAsyncDisconnect is issued, the HIRCLUSTER_FLAG_SHUTDOWN flag will be set, and reconnection can only be achieved via redisClusterConnect2.

Two Options for Reconnection

  1. Issue event_reinit and redisClusterAsyncDisconnect, then use redisClusterConnect2 to clear the shutdown flag and reconnect.
  2. Create a new event_base and asynchronous context, then reconnect to the cluster.

The second option seems preferable.

In either option, the cluster topology will be updated. However, considering that there may still be unissued handlers from the asynchronous CLUSTER NODES command, it seems safer to create a new context entirely.

In option (2.), if there are remaining handlers for the asynchronous CLUSTER NODES, issuing event_base_free could result in dangling pointers.

In either option, To avoid creating dangling pointers, all remaining handlers must be executed. It is necessary to execute event_base_dispatch. Because the HIRCLUSTER_FLAG_SHUTDOWN flag is set, the topology update will not be performed and will return immediately.

Since there’s no significant difference, either method should work.

Add missing event_base_dispatch call after async disconnect to ensure the event loop is properly dispatched and not to create dangling pointers. This change ensures that all pending handlers are processed but all of them will return quiqkly by HIRCLUSTER_FLAG_SHUTDOWN.
Moved the assignment of self->pid closer to re-connection success check in src/Fast.xs. This ensures that the process ID is set only after a successful re-connection attempt.
return;
}

if (redisClusterConnect2(self->acc->cc) != REDIS_OK) {
Copy link
Owner Author

Choose a reason for hiding this comment

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

[MEMO]

I implemented (1.) as it allows for a more simple solution.

Issue event_reinit and redisClusterAsyncDisconnect, then use redisClusterConnect2 to clear the shutdown flag and reconnect.

Since there’s no significant difference, either method should work.

@plainbanana plainbanana merged commit aae0e06 into master Sep 21, 2024
18 checks passed
@plainbanana plainbanana deleted the bump-hiredis-cluster-0140 branch September 21, 2024 10:50
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.

Hangs during the termination process in version 0.091.
1 participant