FluidFramework icon indicating copy to clipboard operation
FluidFramework copied to clipboard

improvement(driver): Make error emission on disconnect optional

Open RishhiB opened this issue 9 months ago • 4 comments

This PR makes the emission of errors on disconnects optional rather than required.

Currently, errors are always emitted on disconnect. However, scenarios like Container.dispose can represent a clean disconnect and shouldn't necessarily trigger an error.

This change introduces an optional error parameter to the dispose and disconnect methods in DocumentDeltaConnection. If an error is provided, it will be used; otherwise, the disconnect is treated as a clean one.

AB#32066

RishhiB avatar May 06 '25 16:05 RishhiB

}

Looks like a breaking change to IDocumentDeltaConnectionEvents disconnect event


Refers to: packages/drivers/driver-base/src/documentDeltaConnection.ts:451 in dbdc5b1. [](commit_id = dbdc5b1aa8411d31137c4c1582fc73c563cb53cf, deletion_comment = False)

jason-ha avatar May 22 '25 19:05 jason-ha

}

Looks like a breaking change to IDocumentDeltaConnectionEvents disconnect event

Refers to: packages/drivers/driver-base/src/documentDeltaConnection.ts:451 in dbdc5b1. [](commit_id = dbdc5b1, deletion_comment = False)

Ah thank you! I hope its correct now.

RishhiB avatar May 26 '25 22:05 RishhiB

This code looks bogus. This appears to be listening for the local "disconnect" event and expecting two parameters. But AFAICT there is only one (reason) which may now be undefined. Am I reading this wrong?


Refers to: packages/drivers/driver-base/src/documentDeltaConnection.ts:641 in b2c5630. [](commit_id = b2c5630103f1b4c688339d0596dbd0172438fae5, deletion_comment = False)

jason-ha avatar Jun 12 '25 23:06 jason-ha

@RishhiB Did someone ask for this change? One is that it is a breaking change and I don't think we can merge it like this as it might break our partners who will be listening to this. Also, previously we were clearly saying in the message that is "client who is closing the connection", so when they would receive that, they would not be doing anything.

jatgarg avatar Jun 26 '25 23:06 jatgarg

@RishhiB Did someone ask for this change? One is that it is a breaking change and I don't think we can merge it like this as it might break our partners who will be listening to this. Also, previously we were clearly saying in the message that is "client who is closing the connection", so when they would receive that, they would not be doing anything.

Yes FRS asked for this. This is the prerequisite for this PR https://github.com/microsoft/FluidFramework/pull/24926, which allows sending disconnect status to FRS. The feature in ADO; https://dev.azure.com/fluidframework/internal/_workitems/edit/8411

RishhiB avatar Jun 30 '25 15:06 RishhiB

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output


> [email protected] ci:check-links /home/runner/work/FluidFramework/FluidFramework/docs
> start-server-and-test "npm run serve -- --no-open" 3000 check-links

1: starting server using command "npm run serve -- --no-open"
and when url "[ 'http://127.0.0.1:3000' ]" is responding with HTTP status code 200
running tests using command "npm run check-links"


> [email protected] serve
> docusaurus serve --no-open

[SUCCESS] Serving "build" directory at: http://localhost:3000/

> [email protected] check-links
> linkcheck http://localhost:3000 --skip-file skipped-urls.txt

Crawling...

Stats:
  231978 links
    1733 destination URLs
    1964 URLs ignored
       0 warnings
       0 errors


github-actions[bot] avatar Jun 30 '25 16:06 github-actions[bot]

@RishhiB Did someone ask for this change? One is that it is a breaking change and I don't think we can merge it like this as it might break our partners who will be listening to this. Also, previously we were clearly saying in the message that is "client who is closing the connection", so when they would receive that, they would not be doing anything.

Yes FRS asked for this. This is the prerequisite for this PR #24926, which allows sending disconnect status to FRS. The feature in ADO; https://dev.azure.com/fluidframework/internal/_workitems/edit/8411

So, if the disconnection is from the socket side, we provide the error. For all other cases, where dispose is coming from the loader, as far as connection is concerned we are just saying that it is the client who is closing this conneciton. I think, we can just create an error type corresponding to the Client Closing delta connection and emit that on connection object and listener would then just check that error type and deduce why the connection get closed instead of undefined.

jatgarg avatar Jun 30 '25 21:06 jatgarg