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 async.c to handle segfaults for async connections #1132

Closed
wants to merge 1 commit into from

Conversation

thisisbasil
Copy link

fixing the amateur hour redis free code

fixing the amateur hour redis free code
@@ -360,6 +360,8 @@ static void __redisRunDisconnectCallback(redisAsyncContext *ac, int status)

/* Helper function to free the context. */
static void __redisAsyncFree(redisAsyncContext *ac) {
if (!ac) return;
Copy link

Choose a reason for hiding this comment

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

When the pointer ac is NULL?

@michael-grunder
Copy link
Collaborator

fixing the amateur hour redis free code

I'm not inclined to merge this PR with that commit message. The fact that it's obnoxious aside, it's useless in a git log as it doesn't say what it's actually doing.

See 21a1207 for a better example.

I'm tagging v1.1.0-rc1 tonight, so feel free to resubmit with a helpful commit message and I will get it merged and into the release.

Comment on lines +418 to +419
if (!ac) return;

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove whitespace.

@@ -360,6 +360,8 @@ static void __redisRunDisconnectCallback(redisAsyncContext *ac, int status)

/* Helper function to free the context. */
static void __redisAsyncFree(redisAsyncContext *ac) {
if (!ac) return;

Copy link
Collaborator

Choose a reason for hiding this comment

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

Whitespace.

@michael-grunder
Copy link
Collaborator

Closing in favor of #1135

@thisisbasil
Copy link
Author

sorry for the tone, this issue caused a problem with production code. good to see it is implemented

@michael-grunder
Copy link
Collaborator

No worries, we've all been there!

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.

3 participants