From 60f8e59fce2cbd7fbfbef16eb91392bae306b7ae Mon Sep 17 00:00:00 2001 From: Cole Miller Date: Sat, 10 Aug 2024 00:13:06 -0400 Subject: [PATCH 1/5] gateway: Break recursion in handle_exec_sql Signed-off-by: Cole Miller --- src/conn.c | 2 +- src/gateway.c | 39 +++++++++++++++++++++++++++++++++++- src/gateway.h | 10 +++++++++ test/unit/test_concurrency.c | 8 ++++++-- test/unit/test_gateway.c | 8 ++++++-- 5 files changed, 61 insertions(+), 6 deletions(-) diff --git a/src/conn.c b/src/conn.c index f6adfa55d..dedadde47 100644 --- a/src/conn.c +++ b/src/conn.c @@ -311,7 +311,7 @@ int conn__start(struct conn *c, c->transport.data = c; c->uv_transport = uv_transport; c->close_cb = close_cb; - gateway__init(&c->gateway, config, registry, raft, seed); + gateway__init(&c->gateway, config, loop, registry, raft, seed); rv = buffer__init(&c->read); if (rv != 0) { goto err_after_transport_init; diff --git a/src/gateway.c b/src/gateway.c index 845e992ed..60b83870d 100644 --- a/src/gateway.c +++ b/src/gateway.c @@ -16,6 +16,7 @@ void gateway__init(struct gateway *g, struct config *config, + uv_loop_t *loop, struct registry *registry, struct raft *raft, struct id_state seed) @@ -34,6 +35,10 @@ void gateway__init(struct gateway *g, g->protocol = DQLITE_PROTOCOL_VERSION; g->client_id = 0; g->random_state = seed; + g->sched = (uv_timer_t){}; + if (loop != NULL) { + uv_timer_init(loop, &g->sched); + } } void gateway__leader_close(struct gateway *g, int reason) @@ -96,6 +101,11 @@ void gateway__leader_close(struct gateway *g, int reason) void gateway__close(struct gateway *g) { tracef("gateway close"); + + if (g->sched.loop != NULL) { + uv_close((uv_handle_t *)&g->sched, NULL); + } + if (g->leader == NULL) { stmt__registry_close(&g->stmts); return; @@ -718,6 +728,25 @@ static void handle_exec_sql_next(struct gateway *g, struct handle *req, bool done); +static void schedule_exec_sql_next_cb(uv_timer_t *t) +{ + struct gateway *g = t->data; + PRE(g->req != NULL); + PRE(g->req->type == DQLITE_REQUEST_EXEC_SQL); + handle_exec_sql_next(g, g->req, true); +} + +static void schedule_exec_sql_next(struct gateway *g) +{ + PRE(g->req->type == DQLITE_REQUEST_EXEC_SQL); + g->sched.data = g; + if (g->sched.loop != NULL) { + uv_timer_start(&g->sched, schedule_exec_sql_next_cb, 0, 0); + } else { + schedule_exec_sql_next_cb(&g->sched); + } +} + static void handle_exec_sql_cb(struct exec *exec, int status) { tracef("handle exec sql cb status %d", status); @@ -728,7 +757,15 @@ static void handle_exec_sql_cb(struct exec *exec, int status) sqlite3_finalize(exec->stmt); if (status == SQLITE_DONE) { - handle_exec_sql_next(g, req, true); + /* The next line simply for the event loop to call + * handle_exec_sql_next on the next iteration. We could just + * call handle_exec_sql_next here, but that causes bounded + * recursion when needsBarrier is false, with the maximum stack + * depth being proportional to the number of `;`-separated + * statements in the original SQL string. We don't want clients + * to be able to trigger this, so we break the cycle here + * instead. */ + schedule_exec_sql_next(g); } else { assert(g->leader != NULL); failure(req, status, error_message(g->leader->conn, status)); diff --git a/src/gateway.h b/src/gateway.h index 2a5805084..b01c95612 100644 --- a/src/gateway.h +++ b/src/gateway.h @@ -35,10 +35,20 @@ struct gateway { uint64_t protocol; /* Protocol format version */ uint64_t client_id; struct id_state random_state; /* For generating IDs */ + /* handle_exec_sql uses this to defer some work to the next loop + * iteration. */ + uv_timer_t sched; }; +/** + * Initialize a gateway. + * + * Passing NULL for the `loop` is permitted. Currently the loop is only used + * optionally to break potential recursion in handle_exec_sql. + */ void gateway__init(struct gateway *g, struct config *config, + uv_loop_t *loop, struct registry *registry, struct raft *raft, struct id_state seed); diff --git a/test/unit/test_concurrency.c b/test/unit/test_concurrency.c index 6008efacc..c3a1afee3 100644 --- a/test/unit/test_concurrency.c +++ b/test/unit/test_concurrency.c @@ -49,8 +49,12 @@ struct connection { struct request_open open; \ struct response_db db; \ struct id_state seed = { { 1 } }; \ - gateway__init(&c->gateway, CLUSTER_CONFIG(0), \ - CLUSTER_REGISTRY(0), CLUSTER_RAFT(0), seed); \ + gateway__init(&c->gateway, \ + CLUSTER_CONFIG(0), \ + NULL, \ + CLUSTER_REGISTRY(0), \ + CLUSTER_RAFT(0), \ + seed); \ c->handle.data = &c->context; \ rc = buffer__init(&c->request); \ munit_assert_int(rc, ==, 0); \ diff --git a/test/unit/test_gateway.c b/test/unit/test_gateway.c index d0e1fb7c6..467cdb1ae 100644 --- a/test/unit/test_gateway.c +++ b/test/unit/test_gateway.c @@ -55,8 +55,12 @@ struct connection { struct id_state seed = { { 1 } }; \ config = CLUSTER_CONFIG(i); \ config->page_size = 512; \ - gateway__init(&c->gateway, config, CLUSTER_REGISTRY(i), \ - CLUSTER_RAFT(i), seed); \ + gateway__init(&c->gateway, \ + config, \ + NULL, \ + CLUSTER_REGISTRY(i), \ + CLUSTER_RAFT(i), \ + seed); \ c->handle.data = &c->context; \ rc = buffer__init(&c->buf1); \ munit_assert_int(rc, ==, 0); \ From 926fbb9ab203d599734ccc7a5a373ddfb1d21c58 Mon Sep 17 00:00:00 2001 From: Cole Miller Date: Sat, 10 Aug 2024 01:08:26 -0400 Subject: [PATCH 2/5] conn: Close gateway in raft_connect This prevents the new timer field from leaking. Signed-off-by: Cole Miller --- src/conn.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/conn.c b/src/conn.c index dedadde47..ea019c1ae 100644 --- a/src/conn.c +++ b/src/conn.c @@ -127,6 +127,7 @@ static void raft_connect(struct conn *c) /* Close the connection without actually closing the transport, since * the stream will be used by raft */ c->closed = true; + gateway__close(&c->gateway); closeCb(&c->transport); } From ceeb4a7b513e340006ef863d41a1f40b60d8897d Mon Sep 17 00:00:00 2001 From: Cole Miller Date: Sat, 10 Aug 2024 00:26:26 -0400 Subject: [PATCH 3/5] Add a test of EXEC_SQL with many statements Signed-off-by: Cole Miller --- test/integration/test_client.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/test/integration/test_client.c b/test/integration/test_client.c index 98fccd428..c2a1cfec2 100644 --- a/test/integration/test_client.c +++ b/test/integration/test_client.c @@ -145,3 +145,29 @@ TEST(client, querySql, setUp, tearDown, 0, client_params) return MUNIT_OK; } + +/* Stress test of an EXEC_SQL with many ';'-separated statements. */ +TEST(client, semicolons, setUp, tearDown, 0, NULL) +{ + struct fixture *f = data; + (void)params; + + static const char trivial_stmt[] = "CREATE TABLE IF NOT EXISTS foo (n INT);"; + + size_t n = 1000; + size_t unit = sizeof(trivial_stmt) - 1; + char *sql = munit_malloc(n * unit); + char *p = sql; + for (size_t i = 0; i < n; i++) { + memcpy(p, trivial_stmt, unit); + p += unit; + } + sql[n * unit - 1] = '\0'; + + uint64_t last_insert_id; + uint64_t rows_affected; + EXEC_SQL(sql, &last_insert_id, &rows_affected); + + free(sql); + return MUNIT_OK; +} From 733843fa9d926baf87f5c90231ff9f7f1a75ed7b Mon Sep 17 00:00:00 2001 From: Cole Miller Date: Tue, 13 Aug 2024 13:46:16 -0400 Subject: [PATCH 4/5] Address review comments Signed-off-by: Cole Miller --- src/gateway.c | 6 +++--- src/gateway.h | 8 +++++--- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/gateway.c b/src/gateway.c index 60b83870d..b7342e7c3 100644 --- a/src/gateway.c +++ b/src/gateway.c @@ -731,7 +731,7 @@ static void handle_exec_sql_next(struct gateway *g, static void schedule_exec_sql_next_cb(uv_timer_t *t) { struct gateway *g = t->data; - PRE(g->req != NULL); + PRE(g != NULL && g->req != NULL); PRE(g->req->type == DQLITE_REQUEST_EXEC_SQL); handle_exec_sql_next(g, g->req, true); } @@ -739,8 +739,8 @@ static void schedule_exec_sql_next_cb(uv_timer_t *t) static void schedule_exec_sql_next(struct gateway *g) { PRE(g->req->type == DQLITE_REQUEST_EXEC_SQL); - g->sched.data = g; if (g->sched.loop != NULL) { + g->sched.data = g; uv_timer_start(&g->sched, schedule_exec_sql_next_cb, 0, 0); } else { schedule_exec_sql_next_cb(&g->sched); @@ -757,7 +757,7 @@ static void handle_exec_sql_cb(struct exec *exec, int status) sqlite3_finalize(exec->stmt); if (status == SQLITE_DONE) { - /* The next line simply for the event loop to call + /* The next line simply arranges for the event loop to call * handle_exec_sql_next on the next iteration. We could just * call handle_exec_sql_next here, but that causes bounded * recursion when needsBarrier is false, with the maximum stack diff --git a/src/gateway.h b/src/gateway.h index b01c95612..d2c915241 100644 --- a/src/gateway.h +++ b/src/gateway.h @@ -35,8 +35,9 @@ struct gateway { uint64_t protocol; /* Protocol format version */ uint64_t client_id; struct id_state random_state; /* For generating IDs */ - /* handle_exec_sql uses this to defer some work to the next loop - * iteration. */ + /* The EXEC_SQL request handler uses this to defer work to the next + * loop iteration, to avoid recursion when processing multi-statement + * SQL strings. */ uv_timer_t sched; }; @@ -44,7 +45,8 @@ struct gateway { * Initialize a gateway. * * Passing NULL for the `loop` is permitted. Currently the loop is only used - * optionally to break potential recursion in handle_exec_sql. + * optionally to break potential recursion when handling an EXEC_SQL request + * that containss multiple `;`-separated statements. */ void gateway__init(struct gateway *g, struct config *config, From 5f69b54c9ad956f78ef0ae985830afb8fc7c9039 Mon Sep 17 00:00:00 2001 From: Cole Miller Date: Tue, 13 Aug 2024 14:47:59 -0400 Subject: [PATCH 5/5] Don't defer handle_exec_sql_next unless necessary Pass the information about whether we have already suspended during handle_exec_sql processing through the leader exec and barrier requests, and use that to only defer the next iteration when handle_exec_sql_next is already on the stack. Signed-off-by: Cole Miller --- src/gateway.c | 50 +++++++++++++++++++++++--------------------------- src/gateway.h | 2 +- src/leader.c | 5 +++++ src/leader.h | 9 ++++++++- 4 files changed, 37 insertions(+), 29 deletions(-) diff --git a/src/gateway.c b/src/gateway.c index b7342e7c3..31320ba1a 100644 --- a/src/gateway.c +++ b/src/gateway.c @@ -35,9 +35,9 @@ void gateway__init(struct gateway *g, g->protocol = DQLITE_PROTOCOL_VERSION; g->client_id = 0; g->random_state = seed; - g->sched = (uv_timer_t){}; + g->defer = (uv_timer_t){}; if (loop != NULL) { - uv_timer_init(loop, &g->sched); + uv_timer_init(loop, &g->defer); } } @@ -102,8 +102,8 @@ void gateway__close(struct gateway *g) { tracef("gateway close"); - if (g->sched.loop != NULL) { - uv_close((uv_handle_t *)&g->sched, NULL); + if (g->defer.loop != NULL) { + uv_close((uv_handle_t *)&g->defer, NULL); } if (g->leader == NULL) { @@ -728,7 +728,7 @@ static void handle_exec_sql_next(struct gateway *g, struct handle *req, bool done); -static void schedule_exec_sql_next_cb(uv_timer_t *t) +static void handle_exec_sql_next_defer_cb(uv_timer_t *t) { struct gateway *g = t->data; PRE(g != NULL && g->req != NULL); @@ -736,17 +736,6 @@ static void schedule_exec_sql_next_cb(uv_timer_t *t) handle_exec_sql_next(g, g->req, true); } -static void schedule_exec_sql_next(struct gateway *g) -{ - PRE(g->req->type == DQLITE_REQUEST_EXEC_SQL); - if (g->sched.loop != NULL) { - g->sched.data = g; - uv_timer_start(&g->sched, schedule_exec_sql_next_cb, 0, 0); - } else { - schedule_exec_sql_next_cb(&g->sched); - } -} - static void handle_exec_sql_cb(struct exec *exec, int status) { tracef("handle exec sql cb status %d", status); @@ -756,20 +745,27 @@ static void handle_exec_sql_cb(struct exec *exec, int status) req->exec_count += 1; sqlite3_finalize(exec->stmt); - if (status == SQLITE_DONE) { - /* The next line simply arranges for the event loop to call - * handle_exec_sql_next on the next iteration. We could just - * call handle_exec_sql_next here, but that causes bounded - * recursion when needsBarrier is false, with the maximum stack - * depth being proportional to the number of `;`-separated - * statements in the original SQL string. We don't want clients - * to be able to trigger this, so we break the cycle here - * instead. */ - schedule_exec_sql_next(g); - } else { + if (status != SQLITE_DONE) { assert(g->leader != NULL); failure(req, status, error_message(g->leader->conn, status)); g->req = NULL; + return; + } + + /* It would be valid to always invoke handle_exec_sql_next directly + * here. But that can lead to bounded recursion when we have several + * `;`-separated statements in a row that do not generate rows. To make + * sure the stack depth stays under control, we defer have the event + * loop invoke handle_exec_sql_next itself on the next iteration, but + * only if there is a prior call to handle_exec_sql_next above us on + * the stack. We also invoke handle_exec_sql_next directly if the + * gateway doesn't have access to an event loop (this is only the case + * in the unit tests). */ + if (exec->async || g->defer.loop == NULL) { + handle_exec_sql_next(g, req, true); + } else { + g->defer.data = g; + uv_timer_start(&g->defer, handle_exec_sql_next_defer_cb, 0, 0); } } diff --git a/src/gateway.h b/src/gateway.h index d2c915241..e3f198990 100644 --- a/src/gateway.h +++ b/src/gateway.h @@ -38,7 +38,7 @@ struct gateway { /* The EXEC_SQL request handler uses this to defer work to the next * loop iteration, to avoid recursion when processing multi-statement * SQL strings. */ - uv_timer_t sched; + uv_timer_t defer; }; /** diff --git a/src/leader.c b/src/leader.c index 61ef91639..0b4a4f23e 100644 --- a/src/leader.c +++ b/src/leader.c @@ -295,6 +295,7 @@ static void leaderApplyFramesCb(struct raft_apply *req, finish: l->inflight = NULL; l->db->tx_id = 0; + l->exec->async = true; leaderExecDone(l->exec); } @@ -439,6 +440,8 @@ static void execBarrierCb(struct barrier *barrier, int status) struct exec *req = barrier->data; struct leader *l = req->leader; + req->async = req->barrier.async; + if (status != 0) { l->exec->status = status; leaderExecDone(l->exec); @@ -505,6 +508,7 @@ static void raftBarrierCb(struct raft_barrier *req, int status) return; } barrier->cb = NULL; + barrier->async = true; cb(barrier, rv); } @@ -514,6 +518,7 @@ int leader__barrier(struct leader *l, struct barrier *barrier, barrier_cb cb) int rv; if (!needsBarrier(l)) { tracef("not needed"); + barrier->async = false; cb(barrier, 0); return 0; } diff --git a/src/leader.h b/src/leader.h index 9d022d3e9..d2ea80602 100644 --- a/src/leader.h +++ b/src/leader.h @@ -49,6 +49,9 @@ struct barrier { void *data; struct leader *leader; struct raft_barrier req; + /* When the callback is invoked, this field is `true` if raft_barrier + * was called and `false` if the callback was invoked immediately. */ + bool async; barrier_cb cb; }; @@ -63,8 +66,12 @@ struct exec { uint64_t id; int status; queue queue; - exec_cb cb; pool_work_t work; + /* When the callback is invoked, this field is `true` if raft_barrier + * or raft_apply was called and `false` if the callback was invoked + * immediately. */ + bool async; + exec_cb cb; }; /**