Add timeout support for libuv adapter #1016

MichaelSuen-thePointer · 2021-11-03T14:35:49Z

see #1015

sorry for no test cases : (

michael-grunder · 2021-11-17T22:00:23Z

Thanks, looks good on first glance. I know it's somewhat trivial but would you mind re-indenting with 4 spaces?

Then I'll get it merged.

MichaelSuen-thePointer · 2021-11-18T06:29:34Z

@michael-grunder
done :)

zuiderkwast

Indentation looks fine. Didn't review the actual code. 🙈

michael-grunder · 2021-12-22T19:04:13Z

Hi, this code asserts when testing against the 0xDEADBEEF sentinel value.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <signal.h>

#include <hiredis.h>
#include <async.h>
#include <adapters/libuv.h>
#include <time.h>
#include <sys/time.h>

void sleepCb(redisAsyncContext *c, void *r, void *privdata) {
    printf("sleep CB\n");
}

int main (int argc, char **argv) {
#ifndef _WIN32
    signal(SIGPIPE, SIG_IGN);
#endif

    uv_loop_t* loop = uv_default_loop();

    redisAsyncContext *c = redisAsyncConnect("127.0.0.1", 6379);
    if (c->err) {
        /* Let *c leak for now... */
        printf("Error: %s\n", c->errstr);
        return 1;
    }

    redisLibuvAttach(c,loop);
    redisAsyncSetTimeout(c, (struct timeval){ .tv_sec = 1, .tv_usec = 0});
    redisAsyncCommand(c, sleepCb, NULL, "DEBUG SLEEP %f", 1.5);
    uv_run(loop, UV_RUN_DEFAULT);
    return 0;
}

$ ./uvtm
sleep CB
uvtm: /home/mike/dev/hiredis/adapters/libuv.h:83: on_handle_close: Assertion `p->timer.data == (void *)0xDEADBEEF' failed.

I'm wondering why we need the sentinel value at all

edit: I took a shot at adding a timeout to the libuv adapter and came up with this.

MichaelSuen-thePointer · 2021-12-26T05:55:05Z

@michael-grunder
I was assuming the timer destroy callback to be earlier than the socket destroy callback, seems I was wrong. I will look into it soon.

edit: I took a shot at adding a timeout to the libuv adapter and came up with this.

I don't know if it is ok to just stop the timer and not 'destroy' it, but it should be good.

MichaelSuen-thePointer · 2021-12-26T06:57:56Z

@michael-grunder
Ok fixed, now I use private data part of the uv handle as a guard to determine whether to call hi_free(p) or not. Now your example passed, and I added it to the example directory.

BTW, It is necessary to call uv_close on all of its handle type.

bjosv

Nice, I have some suggestions.

bjosv · 2022-01-07T10:13:42Z

Makefile

@@ -166,6 +166,8 @@ hiredis-example-macosx: examples/example-macosx.c adapters/macosx.h $(STLIBNAME)
 hiredis-example-ssl: examples/example-ssl.c $(STLIBNAME) $(SSL_STLIBNAME)
 	$(CC) -o examples/$@ $(REAL_CFLAGS) -I. $< $(STLIBNAME) $(SSL_STLIBNAME) $(REAL_LDFLAGS) $(SSL_LDFLAGS)

+hiredis-example-uvtm: examples/example-uvtm.c adapters/libuv.h $(STLIBNAME) $(SSL_STLIBNAME)


We already have an example for libuv some rows below. It might be better to put this new rule close to the existing one?
A suggestion is to rename this new example to something like hiredis-example-libuv-timeout to match the existing naming.
Maybe the existing one can be used for the timeout case?

bjosv · 2022-01-07T10:18:42Z

examples/example-uvtm.c

+#include <sys/time.h>
+
+void sleepCb(redisAsyncContext *c, void *r, void *privdata) {
+    printf("sleep CB\n");


When the async command timeout the reply should be NULL. This can be shown in this example by using

redisReply *reply = r; assert(reply == NULL);

or by some simple printf.

bjosv · 2022-01-07T10:23:29Z

examples/example-uvtm.c

@@ -0,0 +1,42 @@
+


We have an existing example-libuv.c, rename this to something like example-libuv-timeout.c?

bjosv · 2022-01-07T10:29:13Z

Makefile

@@ -166,6 +166,8 @@ hiredis-example-macosx: examples/example-macosx.c adapters/macosx.h $(STLIBNAME)
 hiredis-example-ssl: examples/example-ssl.c $(STLIBNAME) $(SSL_STLIBNAME)
 	$(CC) -o examples/$@ $(REAL_CFLAGS) -I. $< $(STLIBNAME) $(SSL_STLIBNAME) $(REAL_LDFLAGS) $(SSL_LDFLAGS)

+hiredis-example-uvtm: examples/example-uvtm.c adapters/libuv.h $(STLIBNAME) $(SSL_STLIBNAME)


Add the example to the CMakeLists.txt in examples/ folder as well.
See how example-libuv is added and add a ADD_EXECUTABLE and TARGET_LINK_LIBRARIES to the new example.

bjosv · 2022-01-07T10:35:49Z

examples/example-uvtm.c

+    printf("sleep CB\n");
+}
+
+int main (int argc, char **argv) {


warning: unused parameter argc and argv

bjosv · 2022-01-07T10:36:17Z

examples/example-uvtm.c

+#include <time.h>
+#include <sys/time.h>
+
+void sleepCb(redisAsyncContext *c, void *r, void *privdata) {


warning: unused parameter c, r, privdata

…piler warnings

MichaelSuen-thePointer · 2022-01-07T16:56:30Z

push -f to fix commit message, sorry
@bjosv I just merged example-uvtm with example-libuv and removed example-uvtm

michael-grunder · 2022-01-07T18:42:53Z

adapters/libuv.h

+    }
+    // updates the timeout if the timer has already started
+    // or start the timer
+    uv_timer_start(&p->timer, (uv_timer_cb)redisLibuvTimeout, millsec, 0);


Casting a function pointer in this way is undefined behavior and may crash in weird and hard to track down ways.

I'm not overly familiar with libuv but I'm guessing there is a way to #ifdef around it?

Edit: Looks like we can either use UV_VERSION at compile-time, or even uv_version() at runtime and then create a wrapper function with the int parameter that calls our real one without.

Done, it turns out I was using a very old libuv version (0.10.x), however I added the #ifdef and it is ok now.

adapters/libuv.h

Co-authored-by: Michael Grunder <michael.grunder@gmail.com>

zuiderkwast · 2022-01-13T14:59:33Z

adapters/libuv.h

-#if UV_MAJOR_VERSION > 0 || UV_MINOR_VERSION > 11 || (defined(UV_VERSION_PATCH) && UV_VERSION_PATCH > 22)
+#if !defined(UV_VERSION_PATCH) || (UV_MAJOR_VERSION < 1 && UV_MINOR_VERSION < 12 && UV_VERSION_PATCH < 23)
 // libuv removed `status` parameter since v0.11.23


This is maybe wrong for versions like 0.10.37 (it exists).

minor < 12 doesn't always mean minor == 11.

Oh, thanks for the note, you are correct, major.minor.patch < 0.11.23 expands to something like major < 0 || (major == 0 && minor < 11) || (minor == 11 && patch < 23)

Let me try reduce it, could you lend some help?

Major < 0 can't happen so this part can be removed.

I think major == 0 && (minor < 11 || (minor == 11 && patch < 23))

MichaelSuen-thePointer · 2022-01-13T16:15:03Z

Fixed a silly typo, corrected version check macro, thanks @zuiderkwast for the help

adapters/libuv.h

Co-authored-by: Viktor Söderqvist <viktor@zuiderkwast.se>

adapters/libuv.h

michael-grunder · 2022-01-18T18:59:31Z

Merged, thanks!

add timeout support for libuv adapter

e9c245c

michael-grunder self-assigned this Nov 17, 2021

change to 4-space indentation

9a35ee9

zuiderkwast approved these changes Dec 20, 2021

View reviewed changes

fix asserion error in libuv timer support & add example

1039d93

bjosv reviewed Jan 7, 2022

View reviewed changes

merge example-uvtm with example-libuv, add more comments, fix com…

351c1cf

…piler warnings

MichaelSuen-thePointer force-pushed the master branch from 5dc9c81 to 351c1cf Compare January 7, 2022 16:53

michael-grunder reviewed Jan 7, 2022

View reviewed changes

adapt libuv interface change on uv_timer_cb

6403e41

michael-grunder reviewed Jan 11, 2022

View reviewed changes

adapters/libuv.h Outdated Show resolved Hide resolved

change to a cleaner version check macro

e6b4fb2

Co-authored-by: Michael Grunder <michael.grunder@gmail.com>

zuiderkwast reviewed Jan 13, 2022

View reviewed changes

correct version check macro, fix a silly typo

44903ed

zuiderkwast reviewed Jan 13, 2022

View reviewed changes

adapters/libuv.h Outdated Show resolved Hide resolved

Accept suggested version check macro

47a1ec7

Co-authored-by: Viktor Söderqvist <viktor@zuiderkwast.se>

zuiderkwast reviewed Jan 13, 2022

View reviewed changes

adapters/libuv.h Outdated Show resolved Hide resolved

remove outdated comments

1539b7d

michael-grunder merged commit e73ab2f into redis:master Jan 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add timeout support for libuv adapter #1016

Add timeout support for libuv adapter #1016

MichaelSuen-thePointer commented Nov 3, 2021

michael-grunder commented Nov 17, 2021

MichaelSuen-thePointer commented Nov 18, 2021

zuiderkwast left a comment

michael-grunder commented Dec 22, 2021 •

edited

Loading

MichaelSuen-thePointer commented Dec 26, 2021

MichaelSuen-thePointer commented Dec 26, 2021

bjosv left a comment

bjosv Jan 7, 2022

bjosv Jan 7, 2022

bjosv Jan 7, 2022

bjosv Jan 7, 2022

bjosv Jan 7, 2022

bjosv Jan 7, 2022

MichaelSuen-thePointer commented Jan 7, 2022

michael-grunder Jan 7, 2022 •

edited

Loading

MichaelSuen-thePointer Jan 8, 2022

zuiderkwast Jan 13, 2022

MichaelSuen-thePointer Jan 13, 2022

MichaelSuen-thePointer Jan 13, 2022

zuiderkwast Jan 13, 2022

MichaelSuen-thePointer commented Jan 13, 2022

michael-grunder commented Jan 18, 2022

Add timeout support for libuv adapter #1016

Add timeout support for libuv adapter #1016

Conversation

MichaelSuen-thePointer commented Nov 3, 2021

michael-grunder commented Nov 17, 2021

MichaelSuen-thePointer commented Nov 18, 2021

zuiderkwast left a comment

Choose a reason for hiding this comment

michael-grunder commented Dec 22, 2021 • edited Loading

MichaelSuen-thePointer commented Dec 26, 2021

MichaelSuen-thePointer commented Dec 26, 2021

bjosv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelSuen-thePointer commented Jan 7, 2022

michael-grunder Jan 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelSuen-thePointer commented Jan 13, 2022

michael-grunder commented Jan 18, 2022

michael-grunder commented Dec 22, 2021 •

edited

Loading

michael-grunder Jan 7, 2022 •

edited

Loading