redis from mruby #1152

i110 · 2016-12-27T23:59:12Z

This PR makes it able to access redis from mruby hanlder, like the following:

        mruby.handler: |
          redis = H2O::Redis.new(:host => '127.0.0.1', :port => 6379)
          Proc.new do |env|
            set = redis.set('key', 'value')
            get = redis.get('key')
            # redis.disconnect
            [ 200, {}, [set.join, get.join] ]
          end

TODOs:

support more redis features (other commands, authentication, etc..)
consider preparing specific exception class for IO & procotol errors
embed mruby source code in handler/mruby/redis.c ? (I feel it'll reduce maintainability, so let's discuss)
timeout
subscribe

yannick · 2017-01-17T02:15:57Z

if you forget to call .join on the redis answer it segfaults (at least under macOS)

i110 · 2017-01-26T08:33:29Z

@yannick sorry for my late reply, and thank you for noticing me that problem. This is still WIP, and after This PR gets merged, I'll fix it and address FIXMEs.

…edis

yannick · 2017-03-23T23:50:15Z

@redis.connect
  	paste = @redis.call(:hget, "myhash", "field1").join

works fine. however there is a bug when multiple values are sent back from redis:

       @redis.connect
  	paste = @redis.call(:hgetall, "myhash").join

crashes

Assertion failed: (mrb_array_p(*parent)), function try_parentize, file /lib/handler/mruby/redis.c, line 257.

also, is there any reason the connect block is not inside .call?

i110 · 2017-03-27T11:22:45Z

@yannick thank you for notifying me that. I fixed that bug in b100dd5

also, is there any reason the connect block is not inside .call?

no there were no reasons (if I recall correctly), so I moved ensure_connected into call method by 9c6034c

…edis

yannick · 2017-06-09T10:16:29Z

@i110 could you merge master into here, there are a few conflicts i'm unsure about
(also in regards of compat with #1173 )

…edis

yannick · 2017-10-18T11:53:30Z

there might still be a leak:
valgrind reports:
first a empty mruby handler:
https://gist.github.com/yannick/6f957847bb7fa6d88bca12fd1b792d7d
then a handler that calls redis.evalsha( "hash").join):
https://gist.github.com/yannick/e5436edc5aefca1f18510cded573d422

i110 · 2017-10-20T09:58:38Z

Thank you @yannick, I fixed that problem at 70132d8 !

kazuho

Thank you for working on the PR.

I have left my thoughts, mostly on the changes in the C side. To me it seems that several refactors have been made to the redis binding. By asking questions, I am trying to understand why the changes are necessary (or what the benefits are).

kazuho · 2017-11-08T05:22:58Z

include/h2o/mruby_.h

@@ -117,12 +119,14 @@ typedef struct st_h2o_mruby_generator_t {
    } refs;
 } h2o_mruby_generator_t;

+#define H2O_MRUBY_CALLBACK_ID_NOOP 0


I am not sure if 0 as a callback ID is allowed. See https://github.com/h2o/h2o/blob/5b71e0a/lib/handler/mruby.c#L773.

In current implementation, 0 is actually not meaningful and I believe there're no users who actively uses it because it only raises runtime error which means programming error. OTOH we need a way to exit h2o_mruby_run_fiber

kazuho · 2017-11-08T05:25:05Z

lib/common/redis.c

+        return;
+    }
+
+    if (command->type == H2O_REDIS_COMMAND_TYPE_MONITOR) {


Do we need to have code for "monitor" assuming that we always return a error?

kazuho · 2017-11-08T05:29:46Z

lib/common/redis.c

+        return;
+
+    int err = H2O_REDIS_ERROR_NONE;
+    char *errstr = NULL;


Do we need err and errstr?

I would appreciate it if you could merge them instead of having two. One way would be to only use const char * and have specific pointer values defined in the header file. Please consult http1client for details.

At 3ead0da I remoted int err, but still I'm feeling that it may be necessary to report better error message to the client, regarding the fact that error message may came from strerror

kazuho · 2017-11-08T05:32:09Z

lib/common/redis.c

+    h2o_timeout_unlink(entry);
+
+    conn->_did_connect_timeout = 1;
+    invoke_deferred(conn, &conn->_defer_timeout_entry, on_connect_timeout_deferred);


I would apppreciate if you could rather not make deferred calls within timeout callbacks. I believe that there is no reason to do so. Doing more things at once is generally better for performance (better locality) and debugging (gives us better trace).

I believe that there is no reason to do so.

At that time there're some reasons to do workaround with hiredis async API, but at 3ead0da I refactored to eliminate such tricks

kazuho · 2017-11-08T05:39:27Z

lib/common/redis.c

    }
 }

+static void on_disconnect(const redisAsyncContext *redis, int status)


Does the introduction of the function mean that we now have a "disconnecting" (or "closing") state?

If yes, is there a need to add the state to h2o_redis_connection_state_t? If no, why do we need this function?

I would also appreciate it if you could consider unifying the term being used. At the moment I see "close" and "disconnect".

Does the introduction of the function mean that we now have a "disconnecting" (or "closing") state?

In our design, disconnecting hiredis object cannot be touched because once disconnecting procedure is fired our redis client immediately detach hiredis object and try to create new connection if another request comes. Regarding that, I don't think we have to have disconnecting state.

kazuho · 2017-11-08T05:41:41Z

lib/common/redis.c

+{
+    h2o_redis_conn_t *conn = H2O_STRUCT_FROM_MEMBER(h2o_redis_conn_t, _defer_timeout_entry, entry);
+
+    assert((conn->_redis->c.flags & REDIS_CONNECTED) == 0);


Am I correct in assuming that there is a value that represents the connection state in the redisAsyncContext? If so, can we use the value instead of having h2o_redis_connection_state_t?

Having two states that represents the same thing is a source of confusion.

there is a value that represents the connection state in the redisAsyncContext?

Yes, but it's a bit redundant and seems to be not intended to be used directly from outside of hiredis. And also considering that we have a bit different state model described in #1152 (comment), I think we should have our own state representation.

yannick · 2018-01-22T14:13:55Z

it would be great if there would be a way to re-initialize the redis connector if it got killed
or at least we need some example on how this should be handled.

afair: tearing up the redis connection outside of the request handler proc leaves the handler in a non working state if e.g. redis is restarted.

kazuho

Thank you for the changes. I have left my tentative thoughts. Please let me know what you think.

kazuho · 2018-01-29T06:13:33Z

lib/common/redis.c

+    if (h2o_timeout_is_linked(&conn->_connect_timeout_entry))
+        h2o_timeout_unlink(&conn->_connect_timeout_entry);
+    if (h2o_linklist_is_linked(&conn->_connect_timeout._link))
+        h2o_timeout_dispose(conn->loop, &conn->_connect_timeout);


Hasn't _connect_timeout been initialized regardless of if it's has something linked?

In other words, I wonder if it is necessary have a surrounding if when calling the dispose function.

conn->_connect_timeout may be uninitialized if the user set conn->connect_timeout to zero. so we have to check it by h2o_linklist_is_linked(&conn->_connect_timeout._link)

kazuho · 2018-01-29T06:16:25Z

lib/core/util.c

@@ -224,7 +224,7 @@ static h2o_iovec_t build_redis_value(h2o_iovec_t session_data)

 #undef BASE64_LENGTH

-static void redis_resumption_on_get(redisReply *reply, void *_accept_data)
+static void redis_resumption_on_get(redisReply *reply, void *_accept_data, const char *errstr)


Now that we can receive a timeout error through errstr, is it possible to get rid of the timeout handling logic specific to redis-based session resumption that uses on_redis_resumption_get_failed?

If I recall correctly, on_redis_resumption_get_failed is introduced with zero timeout because h2o_socket_ssl_resume_server_handshake or something like that is not reentrant.

kazuho · 2018-01-29T06:31:14Z

lib/handler/mruby/redis.c

+static void on_gc_dispose_redis(mrb_state *mrb, void *_conn)
+{
+    struct st_h2o_mruby_redis_conn_t *conn = _conn;
+    if (conn == NULL) return;


Is there any chance that conn becomes NULL? Unless there's such case, this early return is redundant with the potential of hiding a bug.

fixed at c3d0f85

kazuho · 2018-01-29T06:31:55Z

lib/handler/mruby/redis.c

+static void on_gc_dispose_command(mrb_state *mrb, void *_ctx)
+{
+    struct st_h2o_mruby_redis_command_context_t *ctx = _ctx;
+    if (ctx == NULL) return;


Same as above.

fixed at c3d0f85

kazuho · 2018-01-29T06:44:41Z

include/h2o/redis.h

-    h2o_timeout_entry_t _timeout_entry;
+    h2o_timeout_entry_t _defer_timeout_entry;
+    h2o_timeout_t _connect_timeout;
+    h2o_timeout_entry_t _connect_timeout_entry;


Could you please elaborate the lifecycle of h2o_redis_conn_t::_redis?

I expected that it is set to non-NULL when redisAsyncConnect is called, and is set to NULL when redisAsyncFree is being called.

But that does not seem to be the case. For example, I do not see redisAsyncFree called when on_connect with a status code other than REDIS_OK.

Basically the lifecycle of redis object is managed by hiredis internal. For example, if on_connect gets called with REDIS_ERR, immediately after it hiredis will call __redisAsyncDisconnect which will finally dispose redis object.
https://github.com/h2o/h2o/blob/master/deps/hiredis/async.c#L503-L504

i110 · 2018-01-29T07:29:44Z

@yannick
You can check whether the connection is still alive or not using redis.disconnected? Is this what you need?
OTOH connect method internally checks it, and all redis-command method (like get) calls connect method before emitting commands. So I think the users don't need to use disconnect? method in almost cases.

…back calling code into it

yannick · 2018-01-29T08:18:02Z

@i110 ah right, i forgot that it autoconnects, my bad sorry.

kazuho · 2018-03-07T06:15:20Z

Thank you for your hard work and your patience!

i110 changed the title ~~redis from mruby~~ [WIP] redis from mruby Jan 26, 2017

yannick mentioned this pull request Feb 20, 2017

Build a http web framework? #1205

Open

add mruby redis feature

8d65fa9

i110 force-pushed the i110/mruby-redis branch from 677e956 to 8d65fa9 Compare March 16, 2017 10:16

i110 added 3 commits March 16, 2017 19:23

Merge branch 'i110/mruby-consistent-fiber-handling' into i110/mruby-r…

033a9a1

…edis

tweak for mruby-redis

c41ad6d

fix mruby-redis gc problem

4275123

yannick mentioned this pull request Mar 17, 2017

Distribution yannick/h2o#2

Open

i110 added 3 commits March 27, 2017 17:59

Merge branch 'master' into i110/mruby-redis

da6c09f

call ensure_connected for all commands

9c6034c

fix redis reply allocation problem

b100dd5

i110 added 2 commits March 29, 2017 21:17

Merge branch 'i110/mruby-consistent-fiber-handling' into i110/mruby-r…

a9bfb96

…edis

Merge branch 'i110/mruby-consistent-fiber-handling' into i110/mruby-r…

01b0526

…edis

yannick mentioned this pull request May 3, 2017

Missing delay (sleep, Thread.sleep) in mruby? #1283

Closed

i110 added 11 commits June 9, 2017 19:38

Merge branch 'i110/mruby-consistent-fiber-handling' into i110/mruby-r…

4ebc107

…edis

fix redis reader memory issue

f1d3f2b

tweak redis

5351d1a

raise CommandError when redis command failed

c0eeda0

define redis methods

2e33a36

remove redis reader

b803182

add error code and message in redis command callback

92226db

add redis streaming command

64bc23d

fix mruby redis bugs

c904a00

add redis mruby tests

f486184

install redis-server on travis

fc7e026

i110 added 2 commits October 7, 2017 06:51

fix mruby redis timeout destroying issue

57d3f86

fix mruby-redis command timeout issue

80b8950

i110 force-pushed the i110/mruby-redis branch from dea6d45 to 80b8950 Compare October 6, 2017 21:52

Merge branch 'master' into i110/mruby-redis

66a627d

i110 mentioned this pull request Oct 20, 2017

fix mruby-sleep memory issue #1468

Merged

fix memory leak of mruby-redis

70132d8

kazuho reviewed Nov 8, 2017

View reviewed changes

i110 added 3 commits January 21, 2018 21:08

Merge branch 'master' into i110/mruby-redis

6c8facb

tweak

15c4ef7

remove redis error argument

64b6077

i110 added 3 commits January 26, 2018 22:31

Merge branch 'master' into i110/mruby-redis

ee3cbc0

refactor redis

3ead0da

Merge branch 'i110/asan-symbols' into i110/mruby-redis

a872f9d

kazuho reviewed Jan 29, 2018

View reviewed changes

i110 added 2 commits January 29, 2018 16:52

remove unnecessary NULL checks

c3d0f85

rename close_connection to close_and_detach_connection, and move call…

05fe2a6

…back calling code into it

i110 added 4 commits January 29, 2018 17:30

rename h2o_redis_conn_t to h2o_redis_client_t

0e7f9be

remove unnecessary enum definition

dcad6e5

re-format changes

9460efb

fix heap-use-after-free problem

3c997a9

proyb6 mentioned this pull request Mar 4, 2018

H2O::Redis API #1679

Closed

Merge branch 'master' into i110/mruby-redis

ee0494c

kazuho merged commit 1a99afd into master Mar 7, 2018

utrenkner mentioned this pull request Oct 15, 2018

mruby plans #1881

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

redis from mruby #1152

redis from mruby #1152

redis from mruby #1152

redis from mruby #1152

Conversation

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment