Skip to content

Hiredis: Advanced async-handling#983

Open
oshamash wants to merge 1 commit into
redis:masterfrom
oshamash:omer/async-append-take2
Open

Hiredis: Advanced async-handling#983
oshamash wants to merge 1 commit into
redis:masterfrom
oshamash:omer/async-append-take2

Conversation

@oshamash

Copy link
Copy Markdown
  • hiredis.c: Convert redisSetError to take format and va_args
  • dict.* : Fixed generic hash function -> cannot really take signed number
  • async.* : Valdup function now handles OOM properly
    KeyCompare function uses proper sized integers
    nextArgument - change to use proper str-int conversion function
    Added redisAsyncAppend* function family to support buffer
    optimizations in async-mode
    Added proper error handling on OOM errors [inc buffer rollback]

Review fixes

  • hiredis.c : Reverted back to memcpy(), will test *printf in different time
  • aysnc.c : Minor changes for better readability

Handle unlikely/pathological OOM conditon cleanly.

More OOM checks

@oshamash

Copy link
Copy Markdown
Author

@michael-grunder I took #822 (from #638) and decided to match to upstream, I think it's about time community will have access to ASYNC optimizations using the native hiredis client 😄

Comment thread async.c
Comment thread async.c
Comment thread async.c
Comment thread async.c
Comment thread async.c
@michael-grunder

Copy link
Copy Markdown
Collaborator

I'm going to merge this but first I need to make sure that the change to dictGenHashFunction doesn't somehow cause pain in upstream Redis.

I think it's OK but we've had this kind of symbol conflict before (it's why all the sds symbols in hiredis are prefixed with hi_).

Comment thread async.c Outdated
Comment on lines +327 to +329
while (NULL != (de = dictNext(&it))) \
pcb = dictGetEntryVal(de); \
if (pcb) __redisRunCallback(ac,pcb,NULL); \

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Need a block here.

Suggested change
while (NULL != (de = dictNext(&it))) \
pcb = dictGetEntryVal(de); \
if (pcb) __redisRunCallback(ac,pcb,NULL); \
while (NULL != (de = dictNext(&it))) { \
pcb = dictGetEntryVal(de); \
if (pcb) __redisRunCallback(ac,pcb,NULL); \
} \

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@michael-grunder 👍 good catch, will amend now

@oshamash
oshamash force-pushed the omer/async-append-take2 branch from 088c9ce to b393973 Compare October 3, 2021 08:18
+ hiredis.c: Convert redisSetError to take format and va_args
+ dict.*   : Fixed generic hash function -> cannot really take signed number
+ async.*  : Valdup function now handles OOM properly
             KeyCompare function uses proper sized integers
             nextArgument - change to use proper str-int conversion function
             Added redisAsyncAppend* function family to support buffer
                                                 optimizations in async-mode
             Added proper error handling on OOM errors [inc buffer rollback]

Review fixes

+ hiredis.c : Reverted back to memcpy(), will test *printf in different time
+ aysnc.c   : Minor changes for better readability

Handle unlikely/pathological OOM conditon cleanly.

More OOM checks
@oshamash
oshamash force-pushed the omer/async-append-take2 branch from b393973 to 44f434c Compare October 3, 2021 08:19
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