feat(primer-api): run evaluation requests outside STM #1244
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Prior to this change, we ran API-requested evaluations inside a transaction in STM (via `QueryApp` inside `withSession'`). However, in retrospect, this is (as far as I can determine) completely unnecessary: * Evaluations can take a relatively long time, and could theoretically be restarted if their transaction is aborted; e.g., due to a concurrent edit. * But we don't really care if an edit is made during an evaluation. All we need to do is guarantee that the `App` the evaluation is using is not modified during the evaluation. We can guard against concurrent edits on the `App` used by an evaluation by atomically getting the `App` via STM, returning that app to the API handler for the evaluation request, and then simply running the evaluation on that (immutable) copy of the `App` outside of STM. (Note that we already do something similar in the `availableActions` and `actionOptions` API handlers, for example.) Not only does this change avoid unnecessary evaluation restarts in the event of an aborted transaction, it also means that logging during evaluation is less likely to accumulate large amounts of memory due to the need to use `runPureLog` in STM, since we cannot run IO actions (i.e., log messages) in STM. Speaking of which, the primary motivation for this change is to pave the way for using our new interpreter via the API. Due to its lazy implementation, we're forced to throw exceptions from IO when reporting errors or timeouts in the interpreter, and this would be incompatible with STM. Signed-off-by: Drew Hess <src@drewhess.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prior to this change, we ran API-requested evaluations inside a transaction in STM (via
QueryAppinsidewithSession'). However, in retrospect, this is (as far as I can determine) completely unnecessary:Evaluations can take a relatively long time, and could theoretically be restarted if their transaction is aborted; e.g., due to a concurrent edit.
But we don't really care if an edit is made during an evaluation. All we need to do is guarantee that the
Appthe evaluation is using is not modified during the evaluation.We can guard against concurrent edits on the
Appused by an evaluation by atomically getting theAppvia STM, returning that app to the API handler for the evaluation request, and then simply running the evaluation on that (immutable) copy of theAppoutside of STM. (Note that we already do something similar in theavailableActionsandactionOptionsAPI handlers, for example.)Not only does this change avoid unnecessary evaluation restarts in the event of an aborted transaction, it also means that logging during evaluation is less likely to accumulate large amounts of memory due to the need to use
runPureLogin STM, since we cannot run IO actions (i.e., log messages) in STM.Speaking of which, the primary motivation for this change is to pave the way for using our new interpreter via the API. Due to its lazy implementation, we're forced to throw exceptions from IO when reporting errors or timeouts in the interpreter, and this would be incompatible with STM.