fix: Update UI if custom errorHandler

[caalador]

Enable update of UI for beforeClientResponse
execution throwing an exception when a
custom ErrorHandler is set.

Fixes #17352

[github-actions]

Test Results

1 004 files  ±0  1 004 suites  ±0   1h 3m 22s ⏱️ - 2m 47s
6 394 tests +1  6 353 ✔️ +1  41 💤 ±0  0 ❌ ±0 
6 650 runs  +4  6 602 ✔️ +4  48 💤 ±0  0 ❌ ±0 

Results for commit dd43a73. ± Comparison against base commit 74196a0.

♻️ This comment has been updated with latest results.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[mshabarov]

Not sure that I understand why vaadinSession.getErrorHandler().error(new ErrorEvent(t)); isn't called or doesn't apply UI error-related custom changes when before client reponse callback throws.

When the exception occurs in the callback, it arises from writeUidl to the handleExceptionDuringRequest.

How it makes difference where the exception is thrown, because UIDL write is interrupted anyway if it happens a bit before or later than before client response callbacks handling?

I assume that besides runExecutionsBeforeClientResponse method we have few other places where exceptions potentially can happen that also would break the UIDL writing.

Will do more code digging.

[caalador]

So the normal UIDL request is handled in ServerRpcHandler before write and normal exceptions happen during the invocation handling. UidlRequestHandler::l114 -> ServerRpcHandler::l323 -> ServerRpcHandler::l440 -> ServerRpcHandler::handleInvocationData here you can see that it is handled inside a try catch block that calls the errorHandler and continues execution.
Then when the project throws during the writeUidl when executing beforeClientResponses we will fall all the way up to VaadinService::handleRequest and execute VaadinService::handleExceptionDuringRequest that will run the error handler, but only send the internal error back to the client.

[mshabarov]

Thanks @caalador for explanations, now I think I got it. Looks good to me. Seems like there is nothing else called in writeUidl that might have a user code. JS invocations maybe, but they are not executed on the server anyway.

[tomivirkki]

I wonder if this caused some breaking change? After this was merged, certain EditorImplTest unit tests in flow-components started failing.

[caalador]

Nothing should change if there is no custom ErrorHandler set.
It would seem spreadsheet, upload and grid add one for on attach, so that would make it not show internal error on the client side and no throw as the handler should handle that. I guess its editorIsInBufferedMode_closeEditorThrows that fails?

[tomivirkki]

It's actually those tests that have @Test(expected = IllegalStateException.class):

java.lang.Exception: Unexpected exception, expected[java.lang.IllegalStateException] but was[java.lang.NullPointerException]
Caused by: java.lang.NullPointerException: Cannot invoke "Object.getClass()" because the return value of "com.vaadin.flow.server.VaadinSession.getErrorHandler()" is null

[caalador]

Ok. That's an interesting state, I'll add a null handler test and look into it.

[caalador]

Seems the underlying issue is the mock session that returns a null ErrorHandler at which it fails. Fix incoming.

[caalador]

#17542

[vaadin-bot]

This ticket/PR has been released with Vaadin 24.2.0.alpha10 and is also targeting the upcoming stable 24.2.0 version.