I110/redis session resumption #1087

i110 · 2016-09-26T07:06:40Z

make it able to use Redis as session cache backend.

kazuho

Sorry for the delay.

The design has become clear, and I like the approach. Thank you for all the efforts.

I have left some comments regarding how the binding is implemented. Please let me know what you think.

kazuho · 2016-10-06T05:34:48Z

lib/common/redis/socket.c.h

+
+    ac->ev.data = p;
+#if H2O_USE_LIBUV
+    p->socket = h2o_uv_poll_create(loop, c->fd, (uv_close_cb)free);


You should create a function that calls free instead of trying to pass the address of free, since free might be a macro.

I understood what you intend, and can change it.
BTW, how do you think about other sources which includes this style of passing free callback?
For example, lib/common/multithread.c, lib/common/socket/uv-binding.c.h, and others.
Would it be better to modify these too (in another PR) ?

kazuho · 2016-10-06T05:40:45Z

lib/common/socket/uv-binding.c.h

+        uv_poll_start((uv_poll_t *)sock->handle, sock->poll.events, on_poll);
+        break;
+    default:
+        assert(!"FIXME");


Please use h2o_fatal, since we would not want to see this check dropped when built with -DNDEBUG.

Same applies for other places.

I didn't know that macro. I'll use it.

kazuho · 2016-10-06T05:48:19Z

lib/common/socket/uv-binding.c.h

+    uv_close((uv_handle_t *)poll, (uv_close_cb)free);
+    free(sock);
+    return NULL;
+}


Please do not copy-paste the initialization tasks from h2o_uv_socket_create. Instead, you should extract the logics in common to a separate function and call it from the two create functions.

fmm, I'll try it.

kazuho · 2016-10-06T05:51:54Z

lib/common/redis/socket.c.h

+    p->context = ac;
+
+    return REDIS_OK;
+}


Maybe you should move the contents of this file to lib/common/redis.c? I believe the functions are only used from the file.

Sure, I'll move it.

kazuho · 2016-10-06T05:52:33Z

lib/common/redis.c

+static void on_redis_disconnect(const redisAsyncContext *redis, int status)
+{
+    h2o_redis_conn_t *conn = (h2o_redis_conn_t *)redis->data;
+    if (conn->cb.on_disconnect) {


Please use != NULL considering the fact that on_disconnect is a pointer.

matada. suimasen

kazuho · 2016-10-06T05:54:18Z

lib/common/socket/uv-binding.c.h

        h2o_socket_close(&sock->super);
        return NULL;
    }

    return &sock->super;
 }

+h2o_socket_t *h2o_uv_poll_create(h2o_loop_t *loop, int fd, uv_close_cb close_cb)


Please rename the function to h2o_uv__poll_create considering the fact that the socket create by this function cannot be used with certain API, e.g. h2o_socket_write, h2o_socket_getpeername, etc.

Note: we generally use __ to designate a private function.

kazuho · 2016-10-06T05:58:58Z

include/h2o/redis.h

+#define h2o__redis_h
+
+#include "hiredis.h"
+#include "socket.h"


Is the intention to include "h2o/socket.h"?

soukamo. I'll check it

yannick · 2016-10-07T08:42:14Z

if redis is added it might make sense to look into being able to use it
a) from mruby
b) to store h2o configuration, certificates etc.

especially for things like dynamically choosing the upstream route it would be great.

that being said, i think etcd could be considered as an alternative, it seems to be the de-facto standard now in distributed system for sharing data.

i110 · 2016-10-07T09:51:53Z

@yannick Thank you for sharing your idea.

a) from mruby

Sure, I'm planning to make it able to use redis from mruby handler, in the near future.

b) to store h2o configuration, certificates etc.

As you said, I also believe that etcd is the best for such use case.

especially for things like dynamically choosing the upstream route it would be great.

Sounds great, but could you elaborate more details of your idea? In my first impression, it might be enough to write the routing logic in mruby, using redis as the routing configuration store.

yannick · 2016-10-07T15:35:43Z

especially for things like dynamically choosing the upstream route it would be great.
Sounds great, but could you elaborate more details of your idea? In my first impression, it might be enough to write the routing logic in mruby, using redis as the routing configuration store

it was just an example, and as you mentioned it would be ok for most scenarios to simply lookup the possible upstream hosts in redis. for some scenarios you might want to implement a distribution algorithm directly without the mruby overhead i guess.

i110 · 2016-10-17T08:33:53Z

@yannick Sorry for the delay, and thank you for your suggestion. I believe what you said is worth considering, so I'll revisit it when more specific scenarios appear.

i110 · 2016-10-17T09:01:06Z

TODO: write e2e tests

yannick · 2016-10-17T11:49:20Z

@i110 no worries, i'm only making very high level comments here, and i am not sure in how far they are really relevant here.

from a libh2o's user perspective it would be actually nice if there would be a more clear interface such that adding a different session resumption mechanism would be easier. (e.g. work towards removing hiredis include in ssl.c ). one thing that made nginx very successfull is that its all very modular.

kazuho

Thank you for all your efforts!

I have left mostly high-level comments regarding the design. Please let me know what you think.

kazuho · 2016-10-18T05:21:20Z

include/h2o/redis.h

+#ifndef h2o__redis_h
+#define h2o__redis_h
+
+#include "hiredis.h"


It would be preferable to not introduce dependency at header-file level.

If we keep the dependency at source-code level, it would be easier to build libh2o (or a standalone server) without dependency to hiredis.

the reason why this is included here is, h2o_redis_command_cb uses redisReply as a callback argument (which includes various type of response, like string, array, nil, etc)
Do you mean that we should..

define original struct which represent the response from redis, and copy values to it from redisReply?

typedef redisReply h2o_redis_reply and other constants (like REDIS_REPLY_ERROR) ?

memo: as we talked, we still have to include this here

kazuho · 2016-10-18T05:48:06Z

lib/common/redis.c

+    }
+}
+
+h2o_redis_conn_t *h2o_redis_connect(h2o_loop_t *loop, const char *host, uint16_t port, h2o_redis_callbacks_t cb, void *cb_data)


I see several issues regarding the design of the function.

I. The function returns failure using two methods, either by returning NULL or by calling a callback. Such approach complicates things. I'd suggest always using a callback for notifying errors.
Note that memory allocation in H2O must be done by h2o_mem_alloc (or similar functions defined in h2o/memory.h) that never returns a failure.

II. The function is related to three callbacks: on_connect, on_connection_failure, on_disconnect. I'd suggest merging them into one or two functions (e.g. on_connect and on_close; in which case the latter function would be called for both connection failure and disconnection, with its argument specifying the reason).

III. The interface is designed to use void *cb_data to associate the user data to the redis connection. I'd suggest using structure-extensions instead, much like we do for other structures (e.g. h2o_conn_t, h2o_handler_t). That way, in some applications you would not need to maintain the bidirectional pointer that connects h2o_redis_conn_t and the user data.

IV. The proposed model creates h2o_redis_conn_t upon connection, and discards the object upon disconnection. It might be worth considering if we should decouple connection/disconnection and the lifecycle of the object. My understanding is that in most of the use cases we would want to retain a connection to a redis server, and use it from many places. Having a single object that occasionally connects / disconnects to a redis server might help in that respect.

V. You should be careful in calling the callbacks synchronously. For example, the get callback of async resumption is called from a callback within SSL_handshake. And I believe there is a path that leading h2o_socket_ssl_resume_server_handshake before exiting from the get callback. What this means that SSL_handshake (called from h2o_socket_ssl_resume_server_hanshake) would be called within SSL_handshake (that is calling the get callback). However, SSL_handshake is not a reentrant function. I'd suggest to make all callback invocations asynchronous, by using zero_timeout when necessary.

kazuho · 2016-10-18T05:48:51Z

lib/common/redis.c

+    void *data;
+};
+
+void on_command(redisAsyncContext *redis, void *reply, void *privdata)


Is static missing here?

kazuho · 2016-10-18T05:49:17Z

lib/common/redis.c

+
+/* redis socket adapter */
+
+struct st_redis_socket_data {


Please add the _t suffix.

kazuho · 2016-10-18T05:50:13Z

lib/common/redis.c

+    if (p == NULL) {
+        return -1;
+    }
+    memset(p, 0, sizeof(*p));


As a general rule, I recommend using compound assignment for initializing a structure. In this case, *p = (struct ...){NULL}?

kazuho · 2016-10-18T05:52:41Z

lib/core/util.c

+    struct st_h2o_accept_data_t *accept_data = sock->data;
+    h2o_redis_conn_t *conn = get_redis_connection(accept_data->ctx->ctx);
+    h2o_iovec_t key = base64_encode(session_id, async_resumption_context.redis.prefix);
+    h2o_redis_command(conn, redis_resumption_on_get, accept_data, "GET %s", key.base);


What happens if the get function gets called while hiredis is trying to establish a connection to the server? Doesn't the invocation of on_get callback get postponed until the connection is established (or times out)?

We do not want to delay handshakes. Unless there is a live connection to hiredis, the function should fail immediately.

Doesn't the invocation of on_get callback get postponed until the connection is established (or times out)?

As you worried (and me too), the invocation will be postponed. I'll change the behavior like the following:

when the callback is called, check the connection to the redis (using h2o_redis_is_connected method)

If live connection exists, do the resumption

if not, async resumption fails immediately (and do full-handshake), but try to re-connect to the redis

kazuho · 2016-10-18T05:55:01Z

lib/core/util.c

 }

-void h2o_accept_setup_async_ssl_resumption(h2o_memcached_context_t *memc, unsigned expiration)
+static h2o_iovec_t base64_encode(h2o_iovec_t str, h2o_iovec_t prefix)


Please rename this function to a more appropriate name.

The name doesn't reflect what it is doing: concatenate a prefix with a base64-encoded string.

One possible candidate would be: build_key.

one thing to note is, this function is used to encode the value, not only the key.
I'll try to rename it or split this into two separated functions

kazuho

Thank you for the changes.

The PR looks mostly OK, well-isolated from the existing code-base.

I've left several comments hoping that we could make the resumption-related interface between the core and the protocol binding (e.g. memcached, redis) even clearer.

kazuho · 2016-10-28T06:16:23Z

lib/core/util.c

-    h2o_memcached_req_t *async_resumption_get_req;
+    h2o_timeout_entry_t resumption_failure_timeout;
+    h2o_memcached_req_t *memcached_resumption_get_req;
+    h2o_redis_command_t *redis_resumption_get_command;


How about creating a common structure and inheriting it for memcached and redis? By doing so, I assume you can reduce if statements a lot, and we would also have room to add support for other protocols without making changes to the memcached and redis bindings.

For example, something like below.

struct st_h2o_accept_data_t { h2o_accept_ctx_t *ctx; h2o_socket_t *sock; h2o_timeout_entry_t timeout; struct timeval connected_at; }; struct st_h2o_memcached_accept_data_t { struct st_h2o_accept_data_t super; h2o_memcached_req_t *memcached_resumption_get_req; }; struct st_h2o_redis_accept_data_t { struct st_h2o_accept_data_t super; h2o_timeout_entry_t resumption_failure_timeout; h2o_redis_command_t *redis_resumption_get_command; };

kazuho · 2016-10-28T06:19:05Z

lib/core/util.c

+            h2o_redis_connect(conn, async_resumption_context.redis.host.base, async_resumption_context.redis.port);
+        }
+        // abort resumption
+        h2o_timeout_link(accept_data->ctx->ctx->loop, &accept_data->ctx->ctx->zero_timeout, &accept_data->resumption_failure_timeout);


Would it be possible to reuse st_accept_data_t::timeout instead of introducing resumption_failure_timeout? I assume this is the only use-case for the variable.

kazuho · 2016-10-28T06:24:15Z

src/ssl.c

+    struct {
+        char *host;
+        uint16_t port;
+    } redis;


Can we merge conf.memcached and conf.redis into a union, named address or something?

i110 · 2016-10-28T07:49:28Z

@kazuho thank you for your comment. I addressed the issues.

kazuho

Thank you for the update. Aside from the naming, the code looks good to me.

Let's schedule the release of the redis binding to 2.2.

kazuho · 2016-11-08T14:05:54Z

lib/core/util.c

 }

+static struct {
+    struct st_h2o_accept_data_t *(*create)(h2o_accept_ctx_t *ctx, h2o_socket_t *sock, struct timeval connected_at);
+    void (*free)(struct st_h2o_accept_data_t *accept_data);


Please use destroy instead of free. free is a function defined in ISO C, that might be defined as a macro. In such case, the code will fail to compile.

That is the reason we use destroy as an antonym to create throughout H2O (and FWIW we use dispose as an antonym for init).

kazuho · 2016-11-30T03:23:55Z

Thank you for the updates.

I believe there are no pending issues related to this PR. I'll re-check (and hopefully merge) after we release 2.1.

kazuho

I've gone through the code once more and the only issue I have is the in-line comment.

@i110 Could you please look into it?

kazuho · 2017-01-31T06:02:25Z

lib/common/socket.c

    return 0;
 }

 static void on_async_resumption_remove(SSL_CTX *ssl_ctx, SSL_SESSION *session)
 {
+    if (resumption_remove == NULL) {
+        return;
+    }


Do you recall why the remove callbacks were abolished in edecc08 (for redis) and a5a433b (for memcached)?

If there is some reason to not remove the session tickets, I think it would make more sense for us to make on_async_resumption_remove a no-op rather than relying on resumption_remove == NULL.

@kazuho If I remember correctly, it is because if we use the session backends which have expiration feature, there're no need to remove explicitly the sessions

Thank you for the clarification. Looking at the man page, it says:

The remove_session_cb() is called, whenever the SSL engine removes a session from the internal cache. This happens

when the session is removed because it is expired

or when a connection was not shutdown cleanly.

It also happens for all sessions in the internal session cache when SSL_CTX_free is called.
(source: https://www.openssl.org/docs/man1.0.1/ssl/SSL_CTX_sess_set_remove_cb.html)

So it would be reasonable for an external cache (i.e. a cache that uses memcached or redis) to do nothing when the event gets fired (note also that we would not want to remove a cache entry when a connection is abruptly shut down, which is often the case with the browsers).

To summarize, I support making this change.

OTOH, I do oppose to making this change within this PR. Changing how H2O responds to the remove callback is a bug fix. Therefore, it should go through a separate PR to master, especially since we might want to backport this to 2.0 branch as well.

I would appreciate it if you create a PR against master that fixes the issue. The PR should contain sufficient information describing why the code need to be changed such way.

note also that we would not want to remove a cache entry when a connection is abruptly shut down, which is often the case with the browsers

ah yes, this was the second reason. thank you for the complement.

I would appreciate it if you create a PR against master that fixes the issue.

I'll do it

@kazuho done #1185

…3b1648ef6f1f95 () at deps/hiredis

i110 · 2017-02-06T02:22:26Z

@kazuho I resolved conflicts and rebased.

…929410593f60c1 () at deps/hiredis

kazuho · 2017-10-12T05:48:40Z

I110/redis session resumption #1087

I110/redis session resumption #1087

Uh oh!

Conversation

i110 commented Sep 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kazuho left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

i110 Oct 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kazuho Oct 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kazuho Oct 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yannick commented Oct 7, 2016

Uh oh!

i110 commented Oct 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yannick commented Oct 7, 2016

Uh oh!

i110 commented Oct 17, 2016

Uh oh!

i110 commented Oct 17, 2016

Uh oh!

yannick commented Oct 17, 2016

Uh oh!

kazuho left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

i110 Oct 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

i110 commented Sep 26, 2016 •

edited

Loading

i110 Oct 6, 2016 •

edited

Loading

kazuho Oct 6, 2016 •

edited

Loading

kazuho Oct 6, 2016 •

edited

Loading

i110 commented Oct 7, 2016 •

edited

Loading

i110 Oct 19, 2016 •

edited

Loading

kazuho Oct 28, 2016 •

edited

Loading

i110 Feb 1, 2017 •

edited

Loading