Pass RPC handler exceptions to error handler

[Artur-]

Fixes #801

---

This change is 

[denis-anisimov]

Reviewed 3 of 3 files at r1.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained

a discussion (no related file):
ServerRpcHandler is just one of the handlers.

It's used by UidlRequestHandler which is SynchronizedRequestHandler.
All request handlers are invoked in the VaadinService::handleRequest() method.
So shouldn't we instead catch the exception in a generic way for every request handler in the VaadinService instead of personal catching inside a ServerRpcHandler (and may be a number of other different handlers)?

---

[Artur-]

Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained

a discussion (no related file):

Previously, denis-anisimov (Denis) wrote…

ServerRpcHandler is just one of the handlers.

It's used by UidlRequestHandler which is SynchronizedRequestHandler.
All request handlers are invoked in the VaadinService::handleRequest() method.
So shouldn't we instead catch the exception in a generic way for every request handler in the VaadinService instead of personal catching inside a ServerRpcHandler (and may be a number of other different handlers)?

This is consistent with how FW8 works, wrapping each RPC handler in a separate try-catch block to continue handling other RPCs even though the first might fail. There is a generic handler in VaadinService, that's the one which says "internal error" if nobody else handled the problem. "Internal error" does make sense if this was thrown from some Flow method

---

[denis-anisimov]

Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained

a discussion (no related file):

Previously, Artur- (Artur) wrote…

This is consistent with how FW8 works, wrapping each RPC handler in a separate try-catch block to continue handling other RPCs even though the first might fail. There is a generic handler in VaadinService, that's the one which says "internal error" if nobody else handled the problem. "Internal error" does make sense if this was thrown from some Flow method

No problem with wrapping each RPC handler in a separate try-catch block.

The question is :
should it be

for (RequestHandler handler : getRequestHandlers()) {
     try {
                if (handler.handleRequest(vaadinSession, request, response)) {
                    return;
                }
            }
    }
   catch(Exception e){

      // PROPER EXCEPTION HANDING HERE
   }

instead of having the same code all over every personal handler deep inside as it's done in this PR?

---

[Artur-]

Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained

a discussion (no related file):

Previously, denis-anisimov (Denis) wrote…

No problem with wrapping each RPC handler in a separate try-catch block.

The question is :
should it be

for (RequestHandler handler : getRequestHandlers()) {
     try {
                if (handler.handleRequest(vaadinSession, request, response)) {
                    return;
                }
            }
    }
   catch(Exception e){

      // PROPER EXCEPTION HANDING HERE
   }

instead of having the same code all over every personal handler deep inside as it's done in this PR?

There can be multiple RPC calls inside one request, right? If so, then wrapping the whole request handler would behave differently (remaining RPCs not invoked)

---

[denis-anisimov]

Reviewable status: all discussions resolved, 0 of 1 LGTMs obtained, and 1 stale

a discussion (no related file):

Previously, Artur- (Artur) wrote…

There can be multiple RPC calls inside one request, right? If so, then wrapping the whole request handler would behave differently (remaining RPCs not invoked)

Right.

---

[vaadin-bot]

SonarQube analysis reported 1 issue

Note: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:

1. ServerRpcHandler.java#L404: Move this method into "RpcRequest".

[caalador]

Reviewed 1 of 1 files at r2, 1 of 1 files at r3.
Reviewable status: all discussions resolved, 0 of 1 LGTMs obtained, and 2 stale