canonical / dqlite

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

Break recursion in handle_exec_sql_next (second attempt) #687

Closed cole-miller closed 3 months ago

cole-miller commented 3 months ago

Revised version of the reverted #681, fixing the regression #684. The bug in that PR was of the same type as that fixed in #686: passing a NULL callback when uv_close-ing a handle and then freeing the allocation containing the handle immediately.

To fix this instance of the bug, I put the uv_timer_t in the conn object instead of the gateway, since conn already has an asynchronous closing sequence. This means the gateway goes back to not knowing that it's running on an event loop, which is convenient for the tests.

It's probably most efficient to review this by looking at the diff against the previous PR, as merged:

$ git remote add cole https://github.com/cole-miller/dqlite
$ git fetch cole
$ git diff 7dea935c44a32d1a4006ae23e072aef6f9cce509 cole/exec-sql-suspend-v2
cole-miller commented 3 months ago

Looking into the assertion failures in CI---seems like I didn't account for what happens in the case of a transaction that's split across several EXEC/EXEC_SQL requests.

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 89.70588% with 7 lines in your changes missing coverage. Please review.

Project coverage is 77.96%. Comparing base (2d0d71b) to head (4236cd3). Report is 10 commits behind head on master.

Files Patch % Lines
src/conn.c 88.23% 0 Missing and 4 partials :warning:
src/gateway.c 84.61% 0 Missing and 2 partials :warning:
test/unit/test_concurrency.c 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #687 +/- ## ========================================== + Coverage 77.87% 77.96% +0.09% ========================================== Files 197 197 Lines 27953 28013 +60 Branches 5534 5550 +16 ========================================== + Hits 21768 21840 +72 - Misses 4346 4384 +38 + Partials 1839 1789 -50 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

cole-miller commented 3 months ago

Assertions fixed, it was a silly error. This should be good to go now.