Make getPendingState synchronous when not closing#19517
Merged
anthony-murphy merged 3 commits intoFeb 7, 2024
Conversation
anthony-murphy
commented
Feb 7, 2024
Collaborator
dannimad
reviewed
Feb 7, 2024
| logAndReturnPendingState( | ||
| event, | ||
| getSyncState( | ||
| await this.blobManager.attachAndGetPendingBlobs( |
Contributor
There was a problem hiding this comment.
Really cool way to make the method synchronous!
dannimad
reviewed
Feb 7, 2024
| }); | ||
| }); | ||
|
|
||
| describe("GetPendingState", () => { |
Contributor
There was a problem hiding this comment.
I really like this test pattern for getPendingLocalState. I think we can do something similar to simplify all our stashed op tests
Contributor
There was a problem hiding this comment.
I mean, it won't be a replacement for current stashed op tests but I can see us duplicating some coverage and using it more in subsequent tests
alexvy86
pushed a commit
to alexvy86/FluidFramework
that referenced
this pull request
Feb 13, 2024
## Background This change is in preparation for allowing consumers of fluid to call serialize on a container while a container is in an attaching state, which will include a failed attach where the failure didn't cause the container to close. This will aid in the case of offline, network, or server outage by allowing the application to capture the un-attached state of the container, preserve it locally, and later create a new file from it. AB#5502 ## Overview To capture pending local state while the container is attaching, we will reuse the existing infrastructure which allows capturing pending local state when the container is attached. The missing piece between the detached format and the attached format which is needed for attaching is the pendingRuntimeState. The pendingRuntimeState is returned by the runtime being hosted by the container: [FluidFramework/packages/common/container-definitions/src/runtime.ts#L91](https://github.com/microsoft/FluidFramework/blob/b6589d127f8d1e608ec25e49119d0cb23ac64cdd/packages/common/container-definitions/src/runtime.ts#L91) The current implementation of getPendingLocalState in the ContainerRuntime class is a bit complicated, as it offers two different modes for being called which are determined by the passed in props. The props primarily determine the behavior of dealing with blob uploads, which applications use for things like images and videos. In all cases all pending ops are captured. If the props specify, notifyImminentClosure as true, the ContainerRuntime transitions to an offline state, and short circuits all existing blobs uploads, and waits for their handles to become referenced. This ensures that we can capture both inflight blob contents, and the ops that reference those blob contents within the application’s object model. This is the mode used by the currently experimental closeAndGetPendingState on IContainer. To accomplish this, getPendingLocalState will return a promise that will be complete once all blobs are referenced. There is also a stopBlobAttachingSignal property on the prop object which can be used to stop waiting, and only capture those blobs which have been referenced. When notifyImminentClosure is undefined or false getPendingLocalState ignores inflight blob uploads, and only capture ops and referenced blobs. This is the mode used by the currently experimental getPendingLocalState on IContainer. Today this also returns a promise, but it is not necessary as all the work can be done synchronously. As discussed above, we want to reuse the existing serialize method on IContainer to capture pending state while attaching. This method is currently and should remain synchronous, and we will want to reuse getPendingLocalState without notifyImminentClosure, which currently only does synchronous work, but still returns a promise. 1. Change the ContainerRuntime’s implementation IRuntime.getPendingLocalState to only return a promise if notifyImminentClosure is true and allow synchronous usage via serialize for getting pending ops. 2. Split IRuntime.getPendingLocalState into two methods. One that waits for pending work to finish, and is async, one that synchronously captures complete work. While option 1 increases complexity within the function itself, it does not increase the current API surface area, as it changes behavior only. We can also mitigate this complexity by testing to validate the differing behaviors. This makes option 1 a preferable option, as the current polling-based mechanism for getting pending local state is not the long-term vision. The long-term vision is to move everything to a push-based model that allows incrementally captures local state as it is created. Adding yet another method, as option 2 would require, would further expose and entrench the polling-based model. [AB#7106](https://dev.azure.com/fluidframework/internal/_workitems/edit/7106) --------- Co-authored-by: Tony Murphy <anthonm@microsoft.com>
kian-thompson
pushed a commit
to kian-thompson/FluidFramework
that referenced
this pull request
Feb 13, 2024
## Background This change is in preparation for allowing consumers of fluid to call serialize on a container while a container is in an attaching state, which will include a failed attach where the failure didn't cause the container to close. This will aid in the case of offline, network, or server outage by allowing the application to capture the un-attached state of the container, preserve it locally, and later create a new file from it. AB#5502 ## Overview To capture pending local state while the container is attaching, we will reuse the existing infrastructure which allows capturing pending local state when the container is attached. The missing piece between the detached format and the attached format which is needed for attaching is the pendingRuntimeState. The pendingRuntimeState is returned by the runtime being hosted by the container: [FluidFramework/packages/common/container-definitions/src/runtime.ts#L91](https://github.com/microsoft/FluidFramework/blob/b6589d127f8d1e608ec25e49119d0cb23ac64cdd/packages/common/container-definitions/src/runtime.ts#L91) The current implementation of getPendingLocalState in the ContainerRuntime class is a bit complicated, as it offers two different modes for being called which are determined by the passed in props. The props primarily determine the behavior of dealing with blob uploads, which applications use for things like images and videos. In all cases all pending ops are captured. If the props specify, notifyImminentClosure as true, the ContainerRuntime transitions to an offline state, and short circuits all existing blobs uploads, and waits for their handles to become referenced. This ensures that we can capture both inflight blob contents, and the ops that reference those blob contents within the application’s object model. This is the mode used by the currently experimental closeAndGetPendingState on IContainer. To accomplish this, getPendingLocalState will return a promise that will be complete once all blobs are referenced. There is also a stopBlobAttachingSignal property on the prop object which can be used to stop waiting, and only capture those blobs which have been referenced. When notifyImminentClosure is undefined or false getPendingLocalState ignores inflight blob uploads, and only capture ops and referenced blobs. This is the mode used by the currently experimental getPendingLocalState on IContainer. Today this also returns a promise, but it is not necessary as all the work can be done synchronously. As discussed above, we want to reuse the existing serialize method on IContainer to capture pending state while attaching. This method is currently and should remain synchronous, and we will want to reuse getPendingLocalState without notifyImminentClosure, which currently only does synchronous work, but still returns a promise. 1. Change the ContainerRuntime’s implementation IRuntime.getPendingLocalState to only return a promise if notifyImminentClosure is true and allow synchronous usage via serialize for getting pending ops. 2. Split IRuntime.getPendingLocalState into two methods. One that waits for pending work to finish, and is async, one that synchronously captures complete work. While option 1 increases complexity within the function itself, it does not increase the current API surface area, as it changes behavior only. We can also mitigate this complexity by testing to validate the differing behaviors. This makes option 1 a preferable option, as the current polling-based mechanism for getting pending local state is not the long-term vision. The long-term vision is to move everything to a push-based model that allows incrementally captures local state as it is created. Adding yet another method, as option 2 would require, would further expose and entrench the polling-based model. [AB#7106](https://dev.azure.com/fluidframework/internal/_workitems/edit/7106) --------- Co-authored-by: Tony Murphy <anthonm@microsoft.com>
anthony-murphy
added a commit
that referenced
this pull request
Mar 1, 2024
…19893) ## Background This change is the final change for allowing consumers of fluid to call serialize on a container while a container is in an attaching state, which will include a failed attach where the failure didn't cause the container to close. This will aid in the case of offline, network, or server outage by allowing the application to capture the un-attached state of the container, preserve it locally, and later create a new file from it. [AB#5502](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/5502) ## Overview This change enables calling container.serialize while a container is in the attaching state. It is the culmination of many smaller changes that have gone in to enable this change. - #18829 - #19400 - #19246 - #19517 - #19518 - #19590 - #19634 - #19802 --------- Co-authored-by: Tony Murphy <anthonm@microsoft.com> Co-authored-by: Daniel Madrid <105010181+dannimad@users.noreply.github.com> Co-authored-by: jzaffiro <110866475+jzaffiro@users.noreply.github.com> Co-authored-by: Matt Rakow <ChumpChief@users.noreply.github.com>
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.
Background
This change is in preparation for allowing consumers of fluid to call serialize on a container while a container is in an attaching state, which will include a failed attach where the failure didn't cause the container to close. This will aid in the case of offline, network, or server outage by allowing the application to capture the un-attached state of the container, preserve it locally, and later create a new file from it.
AB#5502
Overview
To capture pending local state while the container is attaching, we will reuse the existing infrastructure which allows capturing pending local state when the container is attached. The missing piece between the detached format and the attached format which is needed for attaching is the pendingRuntimeState. The pendingRuntimeState is returned by the runtime being hosted by the container:
FluidFramework/packages/common/container-definitions/src/runtime.ts#L91
The current implementation of getPendingLocalState in the ContainerRuntime class is a bit complicated, as it offers two different modes for being called which are determined by the passed in props. The props primarily determine the behavior of dealing with blob uploads, which applications use for things like images and videos. In all cases all pending ops are captured.
If the props specify, notifyImminentClosure as true, the ContainerRuntime transitions to an offline state, and short circuits all existing blobs uploads, and waits for their handles to become referenced. This ensures that we can capture both inflight blob contents, and the ops that reference those blob contents within the application’s object model. This is the mode used by the currently experimental closeAndGetPendingState on IContainer. To accomplish this, getPendingLocalState will return a promise that will be complete once all blobs are referenced. There is also a stopBlobAttachingSignal property on the prop object which can be used to stop waiting, and only capture those blobs which have been referenced.
When notifyImminentClosure is undefined or false getPendingLocalState ignores inflight blob uploads, and only capture ops and referenced blobs. This is the mode used by the currently experimental getPendingLocalState on IContainer. Today this also returns a promise, but it is not necessary as all the work can be done synchronously.
As discussed above, we want to reuse the existing serialize method on IContainer to capture pending state while attaching. This method is currently and should remain synchronous, and we will want to reuse getPendingLocalState without notifyImminentClosure, which currently only does synchronous work, but still returns a promise.
synchronously captures complete work.
While option 1 increases complexity within the function itself, it does not increase the current API surface area, as it changes behavior only. We can also mitigate this complexity by testing to validate the differing behaviors. This makes option 1 a preferable option, as the current polling-based mechanism for getting pending local state is not the long-term vision. The long-term vision is to move everything to a push-based model that allows incrementally captures local state as it is created. Adding yet another method, as option 2 would require, would further expose and entrench the polling-based model.
AB#7106