canonical / dqlite

Embeddable, replicated and fault-tolerant SQL engine.
https://dqlite.io
Other
3.83k stars 216 forks source link

gateway: accept interrupt when no query active #482

Closed MathieuBordere closed 1 year ago

MathieuBordere commented 1 year ago

The behavior of an interrupt request was recently changed, previously handling an interrupt when no request was active was basically a no-op, it was changed to a hard error here.

This breaks the package builds of go-dqlite on focal with go 1.13.8, however it doesn't seem to be caught in the go-dqlite CI tests on focal running go 1.13.15.

Decided to revert to previous behavior, unless there's a good reason to change it.

edit: Added a - currently failing - test that actually interrupts a query_sql instead of query. Apparently there's also an issue with the cleanup code that broke around the same time the other behavior broke.

edit2: So if a request comes in on the same connection, req and g->req can point to the same struct handle. This PR saves the current stmt on the interrupt req that's going to be handled instead of overwriting req->stmt (and thus g->req->stmt) with NULL.

edit3: I actually think it's impossible that the same connection object can handle a new request while another one is ongoing. A new request is only read (after the initial protocol message) in the write_cb.

The write_cb can be called during the handling of request in case of a query yielding multiple rows that it needs to send in multiple messages, in that case, gateway__resume will indicate the request is not finished and write_cb will not try to read the next message. write_cb can also be called in case of an exec that resulted in the replication of raft log entries, in that case the write_cb is called after replication is done and the request is finished, so a new request can't be handled during handling of an exec. Same for the other request types, that don't return immediately, we only read a new request after answering the current request.

So, my current understanding is that it's okay to assert in gateway__handle when a request comes in during the handling of another request.

A consequence of this is also that Interrupt doesn't work today, the interrupt request will never be read while the query is yielding rows. The interrupt failure was hit in the go-dqlite tests because the previous implementation crashed when no request was active, and indeed that looks to be the only case that's possible in the current implementation, it works in the unit tests though, because we call gateway__handle directly.

Please correct me if my understanding is wrong.

codecov[bot] commented 1 year ago

Codecov Report

Merging #482 (6950d3c) into master (a856e88) will increase coverage by 0.07%. The diff coverage is 56.25%.

@@            Coverage Diff             @@
##           master     #482      +/-   ##
==========================================
+ Coverage   58.72%   58.80%   +0.07%     
==========================================
  Files          33       33              
  Lines        5917     5916       -1     
  Branches     1778     1776       -2     
==========================================
+ Hits         3475     3479       +4     
+ Misses       1365     1359       -6     
- Partials     1077     1078       +1     
Impacted Files Coverage Δ
src/conn.c 48.52% <0.00%> (ø)
src/gateway.c 43.58% <64.28%> (+0.51%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.