Created a new synchronous http client using RawSynchronousSockets.

Created a new synchronous http client using RawSynchronousSockets.

BUG=
R=zra@google.com

Committed: https://github.com/dart-lang/sync_http/commit/03561155865b40eb7a3853d11ff31d09296c3dc9

[bkonyi, 2017-04-19 16:31:36]

bkonyi@google.com changed reviewers:
+ zra@google.com

[bkonyi, 2017-04-19 16:37:53]

Added quick notes.

https://codereview.chromium.org/2827083002/diff/1/lib/src/http_basic_test.dart
File lib/src/http_basic_test.dart (right):

https://codereview.chromium.org/2827083002/diff/1/lib/src/http_basic_test.dar...
lib/src/http_basic_test.dart:1: // Copyright (c) 2013, the Dart project authors.
 Please see the AUTHORS file
I'll change the copyright date.

https://codereview.chromium.org/2827083002/diff/1/lib/src/http_basic_test.dar...
lib/src/http_basic_test.dart:8: import
"../../../sdk/pkg/expect/lib/expect.dart";
This needs to be changed to not use an explicit path, but not sure what it needs
to change to...

[zra, 2017-04-19 17:46:14]

See the yaml package here: https://github.com/dart-lang/yaml for an idea of the
files and layout to use. Figure out how to run dartfmt and the analyzer on your
package, and make it analyzer and formatter clean.

https://codereview.chromium.org/2827083002/diff/1/codereview.settings
File codereview.settings (right):

https://codereview.chromium.org/2827083002/diff/1/codereview.settings#newcode3
codereview.settings:3: VIEW_VC: https://github.com/dart-lang/sdk/commit/
sdk -> sync_http

https://codereview.chromium.org/2827083002/diff/1/lib/src/http_basic_test.dart
File lib/src/http_basic_test.dart (right):

https://codereview.chromium.org/2827083002/diff/1/lib/src/http_basic_test.dar...
lib/src/http_basic_test.dart:1: // Copyright (c) 2013, the Dart project authors.
 Please see the AUTHORS file
tests should go under the test/ directory.

https://codereview.chromium.org/2827083002/diff/1/lib/src/http_basic_test.dar...
lib/src/http_basic_test.dart:7: import "../sync_http.dart";
This should be a package: import, like:

import "package:sync_http/sync_http.dart";

https://codereview.chromium.org/2827083002/diff/1/lib/src/http_basic_test.dar...
lib/src/http_basic_test.dart:8: import
"../../../sdk/pkg/expect/lib/expect.dart";
On 2017/04/19 16:37:52, bkonyi wrote:
> This needs to be changed to not use an explicit path, but not sure what it
needs
> to change to...

Use the "test" package. Add it under "dev_dependencies" in your pubspec.yaml
file.

https://codereview.chromium.org/2827083002/diff/1/lib/src/http_basic_test.dar...
lib/src/http_basic_test.dart:13: _serverPort = null;
not needed

https://codereview.chromium.org/2827083002/diff/1/lib/src/http_basic_test.dar...
lib/src/http_basic_test.dart:21: var remote = Isolate.spawn(startTestServer,
receivePort.sendPort);
remote is unused

https://codereview.chromium.org/2827083002/diff/1/lib/src/http_basic_test.dar...
lib/src/http_basic_test.dart:50: ReceivePort _statusPort; // Port for receiving
messages from the server.
I think Dart style tends towards putting these at the top of the class.

https://codereview.chromium.org/2827083002/diff/1/lib/src/http_basic_test.dar...
lib/src/http_basic_test.dart:52: var _startedCallback;
You can make a typedef for the function type and use that here instead of 'var'

https://codereview.chromium.org/2827083002/diff/1/lib/src/http_basic_test.dar...
lib/src/http_basic_test.dart:55: class TestServerCommand {
How about an enum?

https://codereview.chromium.org/2827083002/diff/1/lib/src/http_basic_test.dar...
lib/src/http_basic_test.dart:56: static const START = 0;
Check the Dart style guide fro naming constants. Also add 'int'.

https://codereview.chromium.org/2827083002/diff/1/lib/src/http_basic_test.dar...
lib/src/http_basic_test.dart:71: class TestServerStatus {
Maybe split the state off into an enum.

https://codereview.chromium.org/2827083002/diff/1/lib/src/http_basic_test.dar...
lib/src/http_basic_test.dart:199: void testStartStop() {
These tests are asynchronous, and you'll probably want to run them in sequence
rather than in parallel to make them more isolated from each other. Check the
docs of the 'test' package for the right way to run async tests.

https://codereview.chromium.org/2827083002/diff/1/lib/src/line_decoder.dart
File lib/src/line_decoder.dart (right):

https://codereview.chromium.org/2827083002/diff/1/lib/src/line_decoder.dart#n...
lib/src/line_decoder.dart:1: // Copyright 2017 Google Inc. All Rights Reserved.
Please use the usual copyright header.

https://codereview.chromium.org/2827083002/diff/1/lib/src/line_decoder.dart#n...
lib/src/line_decoder.dart:18: const int _LINE_TERMINATOR = 10;
See the Dart style guide for the correct formatting of constants.

https://codereview.chromium.org/2827083002/diff/1/lib/src/sync_http.dart
File lib/src/sync_http.dart (right):

https://codereview.chromium.org/2827083002/diff/1/lib/src/sync_http.dart#newc...
lib/src/sync_http.dart:1: // Copyright 2017 Google Inc. All Rights Reserved.
ditto

https://codereview.chromium.org/2827083002/diff/1/lib/src/sync_http.dart#newc...
lib/src/sync_http.dart:24: class HttpClientSync {
SyncHttpClient

https://codereview.chromium.org/2827083002/diff/1/lib/src/sync_http.dart#newc...
lib/src/sync_http.dart:25: HttpClientRequestSync getUrl(Uri uri) =>
Should these be static? Please add doc comments for public methods here and
below.

https://codereview.chromium.org/2827083002/diff/1/lib/src/sync_http.dart#newc...
lib/src/sync_http.dart:41: class HttpClientRequestSync {
SyncHttpClientReqeust

https://codereview.chromium.org/2827083002/diff/1/lib/src/sync_http.dart#newc...
lib/src/sync_http.dart:42: static const PROTOCOL_VERSION = '1.1';
int

https://codereview.chromium.org/2827083002/diff/1/lib/src/sync_http.dart#newc...
lib/src/sync_http.dart:103: class _HttpClientRequestSyncHeaders implements
HttpHeaders {
SyncHttp...

https://codereview.chromium.org/2827083002/diff/1/lib/src/sync_http.dart#newc...
lib/src/sync_http.dart:328: class HttpClientResponseSync {
SyncHttpClientResponse

https://codereview.chromium.org/2827083002/diff/1/lib/src/sync_http.dart#newc...
lib/src/sync_http.dart:387: !inBody ||
strange indenting

https://codereview.chromium.org/2827083002/diff/1/lib/src/sync_http.dart#newc...
lib/src/sync_http.dart:416: class _HttpClientResponseSyncHeaders implements
HttpHeaders {
SyncHttpClient...

https://codereview.chromium.org/2827083002/diff/1/lib/sync_http.dart
File lib/sync_http.dart (right):

https://codereview.chromium.org/2827083002/diff/1/lib/sync_http.dart#newcode1
lib/sync_http.dart:1: // Copyright 2017 Google Inc. All Rights Reserved.
ditto

[bkonyi, 2017-04-19 21:29:51]

Addressed your comments and fixed analyzer issues. PTAL.

https://codereview.chromium.org/2827083002/diff/1/codereview.settings
File codereview.settings (right):

https://codereview.chromium.org/2827083002/diff/1/codereview.settings#newcode3
codereview.settings:3: VIEW_VC: https://github.com/dart-lang/sdk/commit/
On 2017/04/19 17:46:12, zra wrote:
> sdk -> sync_http

Done.

https://codereview.chromium.org/2827083002/diff/1/lib/src/http_basic_test.dart
File lib/src/http_basic_test.dart (right):

https://codereview.chromium.org/2827083002/diff/1/lib/src/http_basic_test.dar...
lib/src/http_basic_test.dart:1: // Copyright (c) 2013, the Dart project authors.
 Please see the AUTHORS file
On 2017/04/19 16:37:52, bkonyi wrote:
> I'll change the copyright date.

Done.

https://codereview.chromium.org/2827083002/diff/1/lib/src/http_basic_test.dar...
lib/src/http_basic_test.dart:1: // Copyright (c) 2013, the Dart project authors.
 Please see the AUTHORS file
On 2017/04/19 17:46:13, zra wrote:
> tests should go under the test/ directory.

Done.

https://codereview.chromium.org/2827083002/diff/1/lib/src/http_basic_test.dar...
lib/src/http_basic_test.dart:7: import "../sync_http.dart";
On 2017/04/19 17:46:13, zra wrote:
> This should be a package: import, like:
> 
> import "package:sync_http/sync_http.dart";

Tried this before, didn't realize I needed to run pub get to make the library
available. Done.

https://codereview.chromium.org/2827083002/diff/1/lib/src/http_basic_test.dar...
lib/src/http_basic_test.dart:8: import
"../../../sdk/pkg/expect/lib/expect.dart";
On 2017/04/19 17:46:13, zra wrote:
> On 2017/04/19 16:37:52, bkonyi wrote:
> > This needs to be changed to not use an explicit path, but not sure what it
> needs
> > to change to...
> 
> Use the "test" package. Add it under "dev_dependencies" in your pubspec.yaml
> file.

Done.

https://codereview.chromium.org/2827083002/diff/1/lib/src/http_basic_test.dar...
lib/src/http_basic_test.dart:13: _serverPort = null;
On 2017/04/19 17:46:13, zra wrote:
> not needed

Removed.

https://codereview.chromium.org/2827083002/diff/1/lib/src/http_basic_test.dar...
lib/src/http_basic_test.dart:21: var remote = Isolate.spawn(startTestServer,
receivePort.sendPort);
On 2017/04/19 17:46:13, zra wrote:
> remote is unused

Removed.

https://codereview.chromium.org/2827083002/diff/1/lib/src/http_basic_test.dar...
lib/src/http_basic_test.dart:50: ReceivePort _statusPort; // Port for receiving
messages from the server.
On 2017/04/19 17:46:13, zra wrote:
> I think Dart style tends towards putting these at the top of the class.

Good point. Moved.

https://codereview.chromium.org/2827083002/diff/1/lib/src/http_basic_test.dar...
lib/src/http_basic_test.dart:52: var _startedCallback;
On 2017/04/19 17:46:13, zra wrote:
> You can make a typedef for the function type and use that here instead of
'var'

Done.

https://codereview.chromium.org/2827083002/diff/1/lib/src/http_basic_test.dar...
lib/src/http_basic_test.dart:55: class TestServerCommand {
On 2017/04/19 17:46:13, zra wrote:
> How about an enum?

Done.

https://codereview.chromium.org/2827083002/diff/1/lib/src/http_basic_test.dar...
lib/src/http_basic_test.dart:56: static const START = 0;
On 2017/04/19 17:46:13, zra wrote:
> Check the Dart style guide fro naming constants. Also add 'int'.

Replaced with enum.

https://codereview.chromium.org/2827083002/diff/1/lib/src/http_basic_test.dar...
lib/src/http_basic_test.dart:71: class TestServerStatus {
On 2017/04/19 17:46:13, zra wrote:
> Maybe split the state off into an enum.

Done.

https://codereview.chromium.org/2827083002/diff/1/lib/src/http_basic_test.dar...
lib/src/http_basic_test.dart:199: void testStartStop() {
On 2017/04/19 17:46:13, zra wrote:
> These tests are asynchronous, and you'll probably want to run them in sequence
> rather than in parallel to make them more isolated from each other. Check the
> docs of the 'test' package for the right way to run async tests.

Done.

https://codereview.chromium.org/2827083002/diff/1/lib/src/line_decoder.dart
File lib/src/line_decoder.dart (right):

https://codereview.chromium.org/2827083002/diff/1/lib/src/line_decoder.dart#n...
lib/src/line_decoder.dart:1: // Copyright 2017 Google Inc. All Rights Reserved.
On 2017/04/19 17:46:13, zra wrote:
> Please use the usual copyright header.

Done.

https://codereview.chromium.org/2827083002/diff/1/lib/src/line_decoder.dart#n...
lib/src/line_decoder.dart:18: const int _LINE_TERMINATOR = 10;
On 2017/04/19 17:46:13, zra wrote:
> See the Dart style guide for the correct formatting of constants.

Done.

https://codereview.chromium.org/2827083002/diff/1/lib/src/sync_http.dart
File lib/src/sync_http.dart (right):

https://codereview.chromium.org/2827083002/diff/1/lib/src/sync_http.dart#newc...
lib/src/sync_http.dart:1: // Copyright 2017 Google Inc. All Rights Reserved.
On 2017/04/19 17:46:14, zra wrote:
> ditto

Done.

https://codereview.chromium.org/2827083002/diff/1/lib/src/sync_http.dart#newc...
lib/src/sync_http.dart:24: class HttpClientSync {
On 2017/04/19 17:46:13, zra wrote:
> SyncHttpClient

Done.

https://codereview.chromium.org/2827083002/diff/1/lib/src/sync_http.dart#newc...
lib/src/sync_http.dart:25: HttpClientRequestSync getUrl(Uri uri) =>
On 2017/04/19 17:46:14, zra wrote:
> Should these be static? Please add doc comments for public methods here and
> below.

Done.

https://codereview.chromium.org/2827083002/diff/1/lib/src/sync_http.dart#newc...
lib/src/sync_http.dart:41: class HttpClientRequestSync {
On 2017/04/19 17:46:14, zra wrote:
> SyncHttpClientReqeust

Done.

https://codereview.chromium.org/2827083002/diff/1/lib/src/sync_http.dart#newc...
lib/src/sync_http.dart:42: static const PROTOCOL_VERSION = '1.1';
On 2017/04/19 17:46:14, zra wrote:
> int

I'll assume you meant string. Done.

https://codereview.chromium.org/2827083002/diff/1/lib/src/sync_http.dart#newc...
lib/src/sync_http.dart:103: class _HttpClientRequestSyncHeaders implements
HttpHeaders {
On 2017/04/19 17:46:13, zra wrote:
> SyncHttp...

Done.

https://codereview.chromium.org/2827083002/diff/1/lib/src/sync_http.dart#newc...
lib/src/sync_http.dart:328: class HttpClientResponseSync {
On 2017/04/19 17:46:14, zra wrote:
> SyncHttpClientResponse

Done.

https://codereview.chromium.org/2827083002/diff/1/lib/src/sync_http.dart#newc...
lib/src/sync_http.dart:387: !inBody ||
On 2017/04/19 17:46:13, zra wrote:
> strange indenting

For some reason my VIM instance is auto-formatting it to this... I think it's
running dartfmt on file save.

https://codereview.chromium.org/2827083002/diff/1/lib/src/sync_http.dart#newc...
lib/src/sync_http.dart:416: class _HttpClientResponseSyncHeaders implements
HttpHeaders {
On 2017/04/19 17:46:13, zra wrote:
> SyncHttpClient...

Done.

https://codereview.chromium.org/2827083002/diff/1/lib/sync_http.dart
File lib/sync_http.dart (right):

https://codereview.chromium.org/2827083002/diff/1/lib/sync_http.dart#newcode1
lib/sync_http.dart:1: // Copyright 2017 Google Inc. All Rights Reserved.
On 2017/04/19 17:46:14, zra wrote:
> ditto

Done.

[zra, 2017-04-19 22:42:43]

https://codereview.chromium.org/2827083002/diff/20001/lib/src/sync_http.dart
File lib/src/sync_http.dart (right):

https://codereview.chromium.org/2827083002/diff/20001/lib/src/sync_http.dart#...
lib/src/sync_http.dart:35: int get contentLength => hasBody ? _body.length :
null;
Please document all public fields and methods. Here and below.

https://codereview.chromium.org/2827083002/diff/20001/pubspec.yaml
File pubspec.yaml (right):

https://codereview.chromium.org/2827083002/diff/20001/pubspec.yaml#newcode7
pubspec.yaml:7: sdk: '>=1.20.0 <2.0.0'
sync sockets aren't showing up until 1.24, I think?

https://codereview.chromium.org/2827083002/diff/20001/test/http_basic_test.dart
File test/http_basic_test.dart (right):

https://codereview.chromium.org/2827083002/diff/20001/test/http_basic_test.da...
test/http_basic_test.dart:1: // Copyright (c) 2013, the Dart project authors. 
Please see the AUTHORS file
2017

[zra, 2017-04-19 22:43:30]

It would also be nice to set this up with Travis CL, but that can wait until a
subsequent change.

[zra, 2017-04-19 22:43:46]

On 2017/04/19 22:43:30, zra wrote:
> It would also be nice to set this up with Travis CL, but that can wait until a
> subsequent change.

CL -> CI

[bkonyi, 2017-04-20 14:40:48]

Addressed your comments + updated the README. PTAL.

https://codereview.chromium.org/2827083002/diff/20001/lib/src/sync_http.dart
File lib/src/sync_http.dart (right):

https://codereview.chromium.org/2827083002/diff/20001/lib/src/sync_http.dart#...
lib/src/sync_http.dart:35: int get contentLength => hasBody ? _body.length :
null;
On 2017/04/19 22:42:43, zra wrote:
> Please document all public fields and methods. Here and below.

Done (I think I got them all?).

https://codereview.chromium.org/2827083002/diff/20001/pubspec.yaml
File pubspec.yaml (right):

https://codereview.chromium.org/2827083002/diff/20001/pubspec.yaml#newcode7
pubspec.yaml:7: sdk: '>=1.20.0 <2.0.0'
On 2017/04/19 22:42:43, zra wrote:
> sync sockets aren't showing up until 1.24, I think?

Didn't even think of that. Updated.

https://codereview.chromium.org/2827083002/diff/20001/test/http_basic_test.dart
File test/http_basic_test.dart (right):

https://codereview.chromium.org/2827083002/diff/20001/test/http_basic_test.da...
test/http_basic_test.dart:1: // Copyright (c) 2013, the Dart project authors. 
Please see the AUTHORS file
On 2017/04/19 22:42:43, zra wrote:
> 2017

Done.

[zra, 2017-04-20 15:41:40]

Last two small things.

https://codereview.chromium.org/2827083002/diff/40001/README.md
File README.md (right):

https://codereview.chromium.org/2827083002/diff/40001/README.md#newcode5
README.md:5: synchronous HTTP requests.
We should have a warning here similar to the one on RawSynchronousSocket.
Basically, only use this for connections to localhost, and prefer using
dart:io's HttpClient or package:http.

I think the markdown format allows linking to stuff, so you should link to
RawSynchronousSocket, dart:io's HttpClient and package:http.

https://codereview.chromium.org/2827083002/diff/40001/test/http_basic_test.dart
File test/http_basic_test.dart (right):

https://codereview.chromium.org/2827083002/diff/40001/test/http_basic_test.da...
test/http_basic_test.dart:1: // Copyright (c) 2017, the Dart project authors. 
Please see the AUTHORS file
Do you have a test here that sends/receives a lot of data, like ~1MB?

[bkonyi, 2017-04-20 18:07:57]

Added a new test and updated the README. PTAL.

https://codereview.chromium.org/2827083002/diff/40001/README.md
File README.md (right):

https://codereview.chromium.org/2827083002/diff/40001/README.md#newcode5
README.md:5: synchronous HTTP requests.
On 2017/04/20 15:41:40, zra wrote:
> We should have a warning here similar to the one on RawSynchronousSocket.
> Basically, only use this for connections to localhost, and prefer using
> dart:io's HttpClient or package:http.
> 
> I think the markdown format allows linking to stuff, so you should link to
> RawSynchronousSocket, dart:io's HttpClient and package:http.

Updated. I can't link to RawSynchronousSocket's API page yet since it doesn't
exist. Should I just link to the sync_socket.dart file on Github?

https://codereview.chromium.org/2827083002/diff/40001/test/http_basic_test.dart
File test/http_basic_test.dart (right):

https://codereview.chromium.org/2827083002/diff/40001/test/http_basic_test.da...
test/http_basic_test.dart:1: // Copyright (c) 2017, the Dart project authors. 
Please see the AUTHORS file
On 2017/04/20 15:41:40, zra wrote:
> Do you have a test here that sends/receives a lot of data, like ~1MB?

Just added one.

[zra, 2017-04-20 19:10:19]

lgtm

[bkonyi, 2017-04-20 19:18:28]

Description was changed from

==========
Created a new synchronous http client using RawSynchronousSockets.

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

to

==========
Created a new synchronous http client using RawSynchronousSockets.

BUG=
R=zra@google.com

Committed:
https://github.com/dart-lang/sync_http/commit/03561155865b40eb7a3853d11ff31d0...
==========

[bkonyi, 2017-04-20 19:18:29]

Committed patchset #4 (id:60001) manually as
03561155865b40eb7a3853d11ff31d09296c3dc9 (presubmit successful).