Merged
Conversation
justcoon
approved these changes
Jul 23, 2024
noise64
approved these changes
Jul 23, 2024
This file contains hidden or 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
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.
Resolves #665
Solves the issue described in #665 with a different approach than the ticket suggests.
The implemented solution is to treat HTTP requests "atomic" if the following conditions are true:
If these conditions are true, and the execution is interrupted before the whole request is consumed (or the resources are dropped), during recovery we ignore the partially persisted entries and redo the request from the beginning. This is similar to how the
mark_begin_operation/mark_end_operationhost functions work for atomic regions, but with the additional guarantees that it checks for additional interleaved side effects.If the conditions are not met, everything works as before, as in the request is not retried during recovery, and any further read from the response body leads to an error.
This new behavior is implemented in four major steps in this pull request:
BeginRemoteWrite/EndRemoteWriteoplog entries which were previously only used when idempotence mode was turned off, so in case of HTTP requests the are always written to the oplog, no matter what the current idempotence mode is.EndRemoteWriteis emitted. This can happen in various places depending on the client consuming the response body or not. To implement this, ongoing HTTP requests are now tracked in the durable worker context with aHttpRequestState, which tracks the current "owner" that is responsible for writing the end marker. For example ifconsumeis called on the response body, this responsibility is transferred to the input stream. This step required significant changes because to properly implement it, we need to be able to write end markers from resourcedropfunctions. It is not possible to make these drop functions async, so a new synchronisation mechanism was introduced that allows sync host functions to enqueue oplog operations, and all async host functions (which are capable to manipulate the oplog) are awaiting the background processing of these enqueued operations before they execute. This can further refactored later so that all the oplog operations are only available in the returned permit, but this PR is not going that far.WrappedFunctionType, calledRemoteWriteBatched(begin_idx)which allows us to tag all oplog entries which belong to a "batched remote write", such as a HTTP request. They all refer to the oplog index of theBeginRemoteWriteentry emitted at the beginning of the sequence.begin_function(which previously was only doing checks in replay mode when idempotence mode was turned off) can now look ahead to find the matchingEndRemoteWriteentry, and if it cannot, it also checks whether there are any oplog entries which are not belonging to that particular batched operation. If there are not, it uses the same existing technique as the atomic region implementation, jumping to the end of the oplog, and introducing a deleted region for further replays.