Feat/timeout for `q` & `pull`
Adds a timeout to q and pull
Mostly inspired from the timeout approach in datalevin: https://github.com/juji-io/datalevin/blob/master/src/datalevin/timeout.clj (& original PR which adds it)
Is this approach correct, @tonsky ? (seems to work, based on my tests)
Interesting! Can you talk a little when is this useful (I am not saying that it isn’t, just want to understand use-cases)
Implementation-wise:
timeoutns should be probably merged intoutil- I don’t understand why
parse-timeoutis needed. It looks like it supports some legacy formats but we don’t have any legacy? We can make any API we want relation!shouldn’t have exclamation mark in its name (there are no side effects)- I don’t think
(timeout/assert-time-left)should be part ofrelation!. When you do this, it just scatters checks in more or less random places. I’d prefer more deliberate placing. E.g. in-resolve-clause,-collect,aggregate? (binding [timeout/*deadline* (timeout/to-deadlineshould probably be one macro (with-deadline?) that does binding and wraps the body
Sorry just seeing your reply now @tonsky !
The main usecase is when allowing end users to be able to run their own queries. Queries not properly written can cause combinatorial explosion of candidates, and lead either to OOM or to the application freezing for a non-allowably-long period of time (common use case of datascript running in the main thread in browser). In these cases, a timeout allows one to stop the query partway through if the time taken exceeds a value we deem reasonable.
Re: your implementation notes, I will go through them properly next week. BTW, as I mentioned in the PR description, most of your "why"s could be answered by "it was done that way in datalevin, and the relevant parts of the two codebases looked similar enough that I just recreated the same changes here in datascript". I will more properly think about your notes and reply/fix soon though