gracefully handle internal exceptions during request

gracefully handle internal exceptions during request
- new SERVER_ERROR response code
- return SERVER_ERROR response if exception when processing request before request can be sent
- send server.error notification if exception occurs anytime while processing request or future work triggered by request

BUG=dartbug.com/20893
R=brianwilkerson@google.com, paulberry@google.com

Committed: https://code.google.com/p/dart/source/detail?r=40198

[danrubel, 2014-09-11 22:33:51]

danrubel@google.com changed reviewers:
+ brianwilkerson@google.com, paulberry@google.com

[Brian Wilkerson, 2014-09-11 22:48:08]

LGTM

https://codereview.chromium.org/570453002/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/analysis_server.dart (right):

https://codereview.chromium.org/570453002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/analysis_server.dart:408:
_sendServerErrorNotification(exception, stackTrace);
Does this mean that we are sending a notification AND a response with the same
error information? If so, that seems wrong.

[Paul Berry, 2014-09-11 23:04:37]

I agree with Brian's comment about sending both a notification and a response. 
With that addressed, lgtm.

[danrubel, 2014-09-11 23:32:08]

https://codereview.chromium.org/570453002/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/analysis_server.dart (right):

https://codereview.chromium.org/570453002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/analysis_server.dart:408:
_sendServerErrorNotification(exception, stackTrace);
On 2014/09/11 22:48:07, Brian Wilkerson wrote:
> Does this mean that we are sending a notification AND a response with the same
> error information? If so, that seems wrong.

They are different. Notification is structured to include a stack trace and is
standard mechanism for returning internal errors. Response just indicates that
the request failed for some unknown reason. We can remove the response entirely,
but it seemed correct to complete the outstanding request so that the client is
not left hanging.

[Brian Wilkerson, 2014-09-12 01:13:48]

The purpose of the notification is for errors that are not attributable to a
request. If an exception occurs as a result of responding to a request then the
request should be marked as a failure and no other notification should be sent.
I think we want to extend RequestError to include an optional stack trace (or
just use the 'data' field) and then only produce one error.

[danrubel, 2014-09-12 04:57:26]

On 2014/09/12 01:13:48, Brian Wilkerson wrote:
> The purpose of the notification is for errors that are not attributable to a
> request. If an exception occurs as a result of responding to a request then
the
> request should be marked as a failure and no other notification should be
sent.
> I think we want to extend RequestError to include an optional stack trace (or
> just use the 'data' field) and then only produce one error.

Sgtm. I'll land this so that we have better error handling now, and make this
change in a subsequent CL.

[danrubel, 2014-09-12 05:01:53]

Committed patchset #2 (id:20001) manually as 40198 (presubmit successful).

[danrubel, 2014-09-12 19:20:45]

On 2014/09/12 04:57:26, danrubel wrote:
> On 2014/09/12 01:13:48, Brian Wilkerson wrote:
> > The purpose of the notification is for errors that are not attributable to a
> > request. If an exception occurs as a result of responding to a request then
> the
> > request should be marked as a failure and no other notification should be
> sent.
> > I think we want to extend RequestError to include an optional stack trace
(or
> > just use the 'data' field) and then only produce one error.
> 
> Sgtm. I'll land this so that we have better error handling now, and make this
> change in a subsequent CL.

https://codereview.chromium.org/569743003