Send responses for any pending 'analysis.getErrors' requests during context removal.

Send responses for any pending 'analysis.getErrors' requests during context removal.

R=paulberry@google.com
BUG=

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

[Paul Berry, 2014-07-23 15:06:51]

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

https://codereview.chromium.org/417433003/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/analysis_server.dart:59: if (context != null) {
This check shouldn't be necessary.  The caller will only call
applyChangesToContext() on folders that have an associated context.

https://codereview.chromium.org/417433003/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/analysis_server.dart:68: if (context != null) {
Similar situation with this check.

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

https://codereview.chromium.org/417433003/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/domain_analysis.dart:52: response.setResult(ERRORS,
[]);
In this case wouldn't it be more appropriate for the result to be null, since we
couldn't determine what the errors were?

https://codereview.chromium.org/417433003/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/domain_analysis.dart:54: }, test: (e) => e is
String);
This means that in the unlikely event that e is not a string, the response will
never get sent (and we'll have no log messages or assertion failures to help us
debug the problem).

How about something like this instead:

...catchError((message) {
  if (message is! String) {
    AnalysisEngine.instance.logger.logError('Illegal error message during
getErrors: $message');
    message = '';
  }
  Response response = new Response.getErrorsError(request, message);
  ...

https://codereview.chromium.org/417433003/diff/1/pkg/analysis_server/test/ana...
File pkg/analysis_server/test/analysis/get_errors_test.dart (right):

https://codereview.chromium.org/417433003/diff/1/pkg/analysis_server/test/ana...
pkg/analysis_server/test/analysis/get_errors_test.dart:112: return
waitForTasksFinished().then((_) {
Shouldn't just "return _waitForGetErrorsResponse();" be sufficient?

https://codereview.chromium.org/417433003/diff/1/pkg/analysis_server/test/ana...
pkg/analysis_server/test/analysis/get_errors_test.dart:131: waitForResponse() {
It seems unfortunate that we have to busy wait for the response like this.  Is
there any way we could make the tests use MockServerChannel.sendRequest(), which
already contains the necessary logic to wait for the response using conventional
Dart streams?

[scheglov, 2014-07-24 16:54:04]

PTAL

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

https://codereview.chromium.org/417433003/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/analysis_server.dart:59: if (context != null) {
On 2014/07/23 15:06:51, Paul Berry wrote:
> This check shouldn't be necessary.  The caller will only call
> applyChangesToContext() on folders that have an associated context.

We need this in case if "folder removed" watch event happens before some "file
changed" event.
I trigger this situation in the test, because I remove the context before events
for "addFile" are handled.

https://codereview.chromium.org/417433003/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/analysis_server.dart:68: if (context != null) {
On 2014/07/23 15:06:51, Paul Berry wrote:
> Similar situation with this check.

OK

I think we can trust watcher that it won't tell us about removing the same
folder twice unless if it was recreated, so that there is also a corresponding
AnalysisContext.

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

https://codereview.chromium.org/417433003/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/domain_analysis.dart:52: response.setResult(ERRORS,
[]);
On 2014/07/23 15:06:51, Paul Berry wrote:
> In this case wouldn't it be more appropriate for the result to be null, since
we
> couldn't determine what the errors were?

The "errors" field in response is not optional.

https://codereview.chromium.org/417433003/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/domain_analysis.dart:54: }, test: (e) => e is
String);
On 2014/07/23 15:06:51, Paul Berry wrote:
> This means that in the unlikely event that e is not a string, the response
will
> never get sent (and we'll have no log messages or assertion failures to help
us
> debug the problem).
> 
> How about something like this instead:
> 
> ...catchError((message) {
>   if (message is! String) {
>     AnalysisEngine.instance.logger.logError('Illegal error message during
> getErrors: $message');
>     message = '';
>   }
>   Response response = new Response.getErrorsError(request, message);
>   ...

Done.

https://codereview.chromium.org/417433003/diff/1/pkg/analysis_server/test/ana...
File pkg/analysis_server/test/analysis/get_errors_test.dart (right):

https://codereview.chromium.org/417433003/diff/1/pkg/analysis_server/test/ana...
pkg/analysis_server/test/analysis/get_errors_test.dart:112: return
waitForTasksFinished().then((_) {
On 2014/07/23 15:06:51, Paul Berry wrote:
> Shouldn't just "return _waitForGetErrorsResponse();" be sufficient?

Yes, it is sufficient.
Thanks.

https://codereview.chromium.org/417433003/diff/1/pkg/analysis_server/test/ana...
pkg/analysis_server/test/analysis/get_errors_test.dart:131: waitForResponse() {
On 2014/07/23 15:06:51, Paul Berry wrote:
> It seems unfortunate that we have to busy wait for the response like this.  Is
> there any way we could make the tests use MockServerChannel.sendRequest(),
which
> already contains the necessary logic to wait for the response using
conventional
> Dart streams?

OK

Although I have to duplicate some code to simulate a case when the context is
remove between receiving request and analysis finished.

[Paul Berry, 2014-07-24 17:32:04]

lgtm

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

https://codereview.chromium.org/417433003/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/analysis_server.dart:59: if (context != null) {
On 2014/07/24 16:54:04, scheglov wrote:
> On 2014/07/23 15:06:51, Paul Berry wrote:
> > This check shouldn't be necessary.  The caller will only call
> > applyChangesToContext() on folders that have an associated context.
> 
> We need this in case if "folder removed" watch event happens before some "file
> changed" event.
> I trigger this situation in the test, because I remove the context before
events
> for "addFile" are handled.

I believe this won't ever happen in production code, and is unnecessary to unit
test for.  If you look at ContextDirectoryManager._destroyContext(), it cancels
the subscription that is watching for "file changed" events before calling
removeContext().  

But I guess I'm ok with leaving this check in as defensive coding.

[scheglov, 2014-07-24 17:34:11]

Committed patchset #2 manually as r38544 (presubmit successful).