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

Provides an optional cleanup callback for async data. #768

Merged
merged 1 commit into from Apr 10, 2020

Conversation

heronr
Copy link
Contributor

@heronr heronr commented Feb 26, 2020

This change adds a cleanup event to the redisAsyncContext such that the private data can be deleted or modified as a result of the context being free'd. This way it is possible to allocate private data on the context whose lifetime is tied to that of the context.

This is essentially the only meaningful change to hiredis made by hiredis-vip to support cluster. Adding the data cleanup event to hiredis proper allows for hiredis-vip to be a pure wrapper class that depends on hiredis directly. This way hiredis-vip won't continually lag behind.

@michael-grunder
Copy link
Collaborator

I don't have a problem with this.

@mnunberg See any downsides here?

Also you can disregard the failing Windows test, that's not related to your PR.

@heronr
Copy link
Contributor Author

heronr commented Mar 2, 2020

Out of curiosity, @michael-grunder or @mnunberg, are there larger guidelines or suggestions to contributing to hiredis other than blind submitting PRs? I would have liked to have discussed whether this change made sense beforehand but couldn't find the best place to do that.

@michael-grunder
Copy link
Collaborator

There aren't really any explicit guidelines. Larger scale changes often come from requirements in Redis (e.g. RESP3 support), but other than that the library is pretty low velocity.

As for other changes, I tend to try and balance the benefits with how much pain it's going to cause when backported into Redis.

That said, you're welcome to open issues to discuss changes. I'm generally around to respond.

@michael-grunder michael-grunder added this to the 1.0.0 milestone Mar 25, 2020
@michael-grunder michael-grunder changed the title Add an event callback to enable cleaning up the private data on redisAsyncContext Provides an optional cleanup callback for async data. Apr 10, 2020
@michael-grunder michael-grunder merged commit 0184caa into redis:master Apr 10, 2020
@michael-grunder
Copy link
Collaborator

Merged, thanks!

@heronr heronr deleted the async_data_cleanup branch April 10, 2020 06:11
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