feat(dds): do not opaquely encode handles in ops #19242
Conversation
github-actions
bot
added
base: main
connorskees
force-pushed
the
feat/mv-handle-transform
branch
from
2cee998
to
582e50d
Compare
| @@ -451,7 +457,7 @@ export abstract class SharedObjectCore<TEvent extends ISharedObjectEvents = ISha | |||
| this.reSubmit(content, localOpMetadata); | |||
There was a problem hiding this comment.
Should this also decode handles, and then the default implementation re-encodes them?
There was a problem hiding this comment.
Ping @connorskees, @anthony-murphy
There was a problem hiding this comment.
And, would be excellent to get test coverage of roundtripping through these scenarios (resubmit and stashed op, maybe something with rollback)
There was a problem hiding this comment.
this shouldn't matter. the content is the previously submitted content, so it is already parsed. the dds then resubmits anything they want to be submitted which will subsequently get parses in the submit path again
github-actions
bot
added
area: framework
github-actions
bot
added
the
area: framework
This reverts commit 980ac8e.
| @@ -78,3 +78,16 @@ export function createSingleBlobSummary( | |||
| builder.addBlob(key, content); | |||
| return builder.getSummaryTree(); | |||
| } | |||
|
|
|||
| /** | |||
| * Binds all handles found in `value` to `bind`. Does not modify original input. | |||
There was a problem hiding this comment.
| * Binds all handles found in `value` to `bind`. Does not modify original input. | |
| * Binds all IFluidHandles found in `value` to `bind`. Does not modify original input. |
|
|
||
| // Set the value locally. | ||
| const previousValue = this.setCore(key, localValue, true); | ||
|
|
||
| // If we are not attached, don't submit the op. | ||
| if (!this.isAttached()) { | ||
| bindHandles(localValue.value, this.serializer, this.handle); |
There was a problem hiding this comment.
Add a comment here why this extra step is needed. Here and other places where this is used.
|
@connorskees @markfields This is a risky change. We should ensure that are enough tests in place to not break anything. The scenario where a combination of new and old runtime version clients are running in parallel should be validated. We should add basic end-to-end tests where there are 2 containers - one writes and other reads and run it in full compat mode so it does the mutli-client version compat testing. |
|
@agarwal-navin -- I agree it is risky. Without any type validation to distinguish between encoded v. decoded handles, it's nearly impossible to tell whether all the flips/flops between those states have been addressed correctly. In fact, I know of one still in the PR that hasn't, I've logged a follow-up item. The reason I'm not overly concerned is that functions like So as for this:
I think we have a pretty good argument that there's no change over the wire. That said, better safe than sorry, so the test you suggested is a good idea. I wonder if we can find an existing test that is doing this incidentally? @connorskees what do you think? |
|
(Disabling auto-merge until @connorskees has a chance to respond to @agarwal-navin) |
connorskees
force-pushed
the
feat/mv-handle-transform
branch
from
733477a
to
f7cdc56
Compare
There was a problem hiding this comment.
I looked at the tree changes only in latest revision. they look good. We could do further cleanup in experimental tree if we wanted to since the serializer/deserializer abstraction gets plumbed to more layers, but totally fine to push that out of this PR as is done.
Reverts #19242 due to a bug where duplicate attach ops are sent because of the change to encoding handles.
…code (#20995) Adds end to end tests to validate that each DDS does not send duplicate attach ops when handles are encoded. This bug was added in #19242, and deals with a second attach op being sent when there is a handle from one detached DDS to another and the DDS is then attached. [AB#7936](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/7936) --------- Co-authored-by: Mark Fields <markfields@users.noreply.github.com> Co-authored-by: Tony Murphy <anthony.murphy@microsoft.com>
…code (microsoft#20995) Adds end to end tests to validate that each DDS does not send duplicate attach ops when handles are encoded. This bug was added in microsoft#19242, and deals with a second attach op being sent when there is a handle from one detached DDS to another and the DDS is then attached. [AB#7936](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/7936) --------- Co-authored-by: Mark Fields <markfields@users.noreply.github.com> Co-authored-by: Tony Murphy <anthony.murphy@microsoft.com>
Fixes #8016. We need to port this back to RC3 and RC4 once merged to main. For anyone implementing their own DDS, when they pick up an FF release with #19242 in it, they will likely be in a position where handles are going through `parseHandles` or `serializer.decode` twice. We assumed this was idempotent but found that it is not, with two different codepaths needing to be fixed. Note that `parseHandles` may return the same object unmodified now, wherease before it would always clone (via `JSON.stringify` roundtrip) --------- Co-authored-by: Alex Villarreal <716334+alexvy86@users.noreply.github.com>
Fixes microsoft#8016. We need to port this back to RC3 and RC4 once merged to main. For anyone implementing their own DDS, when they pick up an FF release with microsoft#19242 in it, they will likely be in a position where handles are going through `parseHandles` or `serializer.decode` twice. We assumed this was idempotent but found that it is not, with two different codepaths needing to be fixed. Note that `parseHandles` may return the same object unmodified now, wherease before it would always clone (via `JSON.stringify` roundtrip) --------- Co-authored-by: Alex Villarreal <716334+alexvy86@users.noreply.github.com>
Fixes microsoft#8016. We need to port this back to RC3 and RC4 once merged to main. For anyone implementing their own DDS, when they pick up an FF release with microsoft#19242 in it, they will likely be in a position where handles are going through `parseHandles` or `serializer.decode` twice. We assumed this was idempotent but found that it is not, with two different codepaths needing to be fixed. Note that `parseHandles` may return the same object unmodified now, wherease before it would always clone (via `JSON.stringify` roundtrip) --------- Co-authored-by: Alex Villarreal <716334+alexvy86@users.noreply.github.com>
Fixes microsoft#8016. We need to port this back to RC3 and RC4 once merged to main. For anyone implementing their own DDS, when they pick up an FF release with microsoft#19242 in it, they will likely be in a position where handles are going through `parseHandles` or `serializer.decode` twice. We assumed this was idempotent but found that it is not, with two different codepaths needing to be fixed. Note that `parseHandles` may return the same object unmodified now, wherease before it would always clone (via `JSON.stringify` roundtrip) --------- Co-authored-by: Alex Villarreal <716334+alexvy86@users.noreply.github.com>
Update DDSes to not opaquely encode handles as strings, but rather pass around actual objects.
AB#6382