Add a diagnostic.getServerPort analysis server request.

Add a diagnostic.getServerPort analysis server request.

BUG=
R=brianwilkerson@google.com, scheglov@google.com

Committed: https://github.com/dart-lang/sdk/commit/7f83d9d173c7b90395579166f007a315704d1483

[devoncarew, 2017-02-18 19:26:36]

devoncarew@google.com changed reviewers:
+ brianwilkerson@google.com, scheglov@google.com

[devoncarew, 2017-02-18 19:26:36]

Add a diagnostic.getServerPort analysis server request (fix
https://github.com/dart-lang/sdk/issues/25395).

- create diagnostic.getServerPort
- add an integration test
- regen the protocol lib and docs
- on an error (likely an issue binding to a port), return a new code,
ERROR_HANDLING_REQUEST
- some slight refactoring to allow the diagnostic domain have the right
references to be able to start the diagnostic server

[scheglov, 2017-02-18 20:28:40]

lgtm

[Brian Wilkerson, 2017-02-18 21:41:03]

lgtm once comments are addressed

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

https://codereview.chromium.org/2703033002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/domain_diagnostic.dart:32: static const String
GET_SERVER_PORT = 'diagnostic.getServerPort';
The rest of the constants like this are in the file
"pkg/analysis_server/lib/src/constants.dart". It would be nice to keep them all
together and use a consistent naming scheme.

https://codereview.chromium.org/2703033002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/domain_diagnostic.dart:117: Response
getServerPort(Request request) {
Convert this to use an async body? I find it to be much more readable.

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

https://codereview.chromium.org/2703033002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/server/http_server.dart:133: // connection (as well
as the diagnostics page over http); is this intended?
It's there for historic reasons. The original plan was for analysis server to
connect over http, but then we decided that stdio would be more secure and
faster. The old code has never been cleaned up.

https://codereview.chromium.org/2703033002/diff/1/pkg/analysis_server/tool/sp...
File pkg/analysis_server/tool/spec/spec_input.html (right):

https://codereview.chromium.org/2703033002/diff/1/pkg/analysis_server/tool/sp...
pkg/analysis_server/tool/spec/spec_input.html:9: <h1
style="color:#999999">Version <version>1.17.0</version></h1>
We need to increment the version number (to 1.18.0), both here and in the code
so that clients can reliably use the version number to determine whether the new
request is supported.

https://codereview.chromium.org/2703033002/diff/1/pkg/analysis_server/tool/sp...
pkg/analysis_server/tool/spec/spec_input.html:3856:
<code>ERROR_HANDLING_REQUEST</code>
This is too general; all existing and future errors could be replaced by this.
:-) Error codes are more useful if they carry some semantic value. (Otherwise
there's no point in having an error code.) Perhaps this could be something like
"DEBUG_PORT_COULD_NOT_BE_OPENED".

Also, we keep enum values alphabetized in their lists.

[devoncarew, 2017-02-19 02:43:05]

I think I addressed all the comments! ptal

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

https://codereview.chromium.org/2703033002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/domain_diagnostic.dart:32: static const String
GET_SERVER_PORT = 'diagnostic.getServerPort';
On 2017/02/18 21:41:03, Brian Wilkerson wrote:
> The rest of the constants like this are in the file
> "pkg/analysis_server/lib/src/constants.dart". It would be nice to keep them
all
> together and use a consistent naming scheme.

Good to know; moved these into constants.dart and updated their names.

https://codereview.chromium.org/2703033002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/domain_diagnostic.dart:117: Response
getServerPort(Request request) {
On 2017/02/18 21:41:03, Brian Wilkerson wrote:
> Convert this to use an async body? I find it to be much more readable.

Sure, done.

As an aside, I'm wondering if the `Response handleRequest(Request request)`
handlers for the domains should all be async (return Future<Response>)? Many of
the requests seems async in nature.

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

https://codereview.chromium.org/2703033002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/server/http_server.dart:133: // connection (as well
as the diagnostics page over http); is this intended?
On 2017/02/18 21:41:03, Brian Wilkerson wrote:
> It's there for historic reasons. The original plan was for analysis server to
> connect over http, but then we decided that stdio would be more secure and
> faster. The old code has never been cleaned up.

Makes sense. I updated the comment a bit to mention this.

https://codereview.chromium.org/2703033002/diff/1/pkg/analysis_server/tool/sp...
File pkg/analysis_server/tool/spec/spec_input.html (right):

https://codereview.chromium.org/2703033002/diff/1/pkg/analysis_server/tool/sp...
pkg/analysis_server/tool/spec/spec_input.html:9: <h1
style="color:#999999">Version <version>1.17.0</version></h1>
On 2017/02/18 21:41:03, Brian Wilkerson wrote:
> We need to increment the version number (to 1.18.0), both here and in the code
> so that clients can reliably use the version number to determine whether the
new
> request is supported.

Updated to 1.8.0. Do we have a notion of a pre-release change? Not sure if there
would be more changes we'd want to batch up.

https://codereview.chromium.org/2703033002/diff/1/pkg/analysis_server/tool/sp...
pkg/analysis_server/tool/spec/spec_input.html:3856:
<code>ERROR_HANDLING_REQUEST</code>
On 2017/02/18 21:41:03, Brian Wilkerson wrote:
> This is too general; all existing and future errors could be replaced by this.
> :-) Error codes are more useful if they carry some semantic value. (Otherwise
> there's no point in having an error code.) Perhaps this could be something
like
> "DEBUG_PORT_COULD_NOT_BE_OPENED".
> 
> Also, we keep enum values alphabetized in their lists.

:) It did seem a bit general when I was writing it.
DEBUG_PORT_COULD_NOT_BE_OPENED sgtm; updated, and adjusted the position to sort
correctly.

[Brian Wilkerson, 2017-02-19 16:47:18]

lgtm

> I'm wondering if the `Response handleRequest(Request request)`
> handlers for the domains should all be async (return
> Future<Response>)? Many of the requests seems async in nature.

Maybe. But I'd like to wait until we see how the plugin support changes things
before deciding that.

> Updated to 1.8.0. Do we have a notion of a pre-release change?

We don't. As soon as we commit a change there's a possibility that it will be
part of the next dev build. Also, we can't know that users aren't using builds
built from bleeding edge (I do, all the time). Therefore, it seems like any
build of server must be correct (in terms of version numbers). If we really want
to batch things up then I think we have to create a branch and integrate all the
changes back onto bleeding edge in one atomic merge operation.

> Not sure if there would be more changes we'd want to batch up.

There are several requested changes that I would like to implement, but I
haven't had (and still don't have) time to do so.

https://codereview.chromium.org/2703033002/diff/20001/pkg/analysis_server/too...
File pkg/analysis_server/tool/spec/spec_input.html (right):

https://codereview.chromium.org/2703033002/diff/20001/pkg/analysis_server/too...
pkg/analysis_server/tool/spec/spec_input.html:2056: this call will return an
error of DEBUG_PORT_COULD_NOT_BE_OPENED.
nit: everywhere else we've surrounded the name of the error with "<tt>" and it
would be good to be consistent

https://codereview.chromium.org/2703033002/diff/20001/pkg/analysis_server/too...
pkg/analysis_server/tool/spec/spec_input.html:3749: The server was unable to a
port for the diagnostic server.
"to a" --> "to open a"

[scheglov, 2017-02-19 19:39:53]

LGTM

[devoncarew, 2017-02-19 23:17:39]

> > Updated to 1.8.0. Do we have a notion of a pre-release change?
>
>We don't. As soon as we commit a change there's a possibility that it will be
> part of the next dev build. Also, we can't know that users aren't using builds

Sounds good. One possibility for the future is to use a -dev suffix; to rev the
version, append dev, and plan on batching changes up during the course of a
general 6-week release cycle. Not important for now, but something to think
about when we have a few API changes in the works.

https://codereview.chromium.org/2703033002/diff/20001/pkg/analysis_server/too...
File pkg/analysis_server/tool/spec/spec_input.html (right):

https://codereview.chromium.org/2703033002/diff/20001/pkg/analysis_server/too...
pkg/analysis_server/tool/spec/spec_input.html:2056: this call will return an
error of DEBUG_PORT_COULD_NOT_BE_OPENED.
On 2017/02/19 16:47:18, Brian Wilkerson wrote:
> nit: everywhere else we've surrounded the name of the error with "<tt>" and it
> would be good to be consistent

Done.

https://codereview.chromium.org/2703033002/diff/20001/pkg/analysis_server/too...
pkg/analysis_server/tool/spec/spec_input.html:3749: The server was unable to a
port for the diagnostic server.
On 2017/02/19 16:47:18, Brian Wilkerson wrote:
> "to a" --> "to open a"

Done.

[devoncarew, 2017-02-19 23:19:38]

Description was changed from

==========
Add a diagnostic.getServerPort analysis server request.

BUG=
==========

to

==========
Add a diagnostic.getServerPort analysis server request.

BUG=
R=brianwilkerson@google.com, scheglov@google.com

Committed:
https://github.com/dart-lang/sdk/commit/7f83d9d173c7b90395579166f007a315704d1483
==========

[devoncarew, 2017-02-19 23:19:40]

Committed patchset #3 (id:40001) manually as
7f83d9d173c7b90395579166f007a315704d1483 (presubmit successful).