Add support for loading scripts from http

Add support for loading scripts from http

R=iposva@google.com

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

[Cutch, 2013-05-24 03:00:37]

Add support to the command line VM to:

* Load a script over HTTP. e.g. dart http://localhost:8000/test.dart
* Import a library over HTTP. e.g. import 'http://localhost:8000/mylib.dart';
* Generate a snapshot over HTTP. e.g. --generate-script-snapshot=test.snapshot
http://127.0.0.1:8000/test.dart

There is a remaining issue with DNS resolve failing.
I think that the dart:io HttpClient should be used instead of my hand rolled
(and far less tested) http get function.

[Cutch, 2013-05-24 16:43:30]

Patch Set 4 fixes the DNS resolve issue for localhost.
Tested against a Dart file hosted on code.google.com

[Cutch, 2013-05-24 17:01:09]

Filed this bug about synchronous http request support in dart:io:
https://code.google.com/p/dart/issues/detail?id=10856

[Ivan Posva, 2013-05-24 17:53:43]

https://codereview.chromium.org/15966002/diff/9001/runtime/bin/bin.gypi
File runtime/bin/bin.gypi (right):

https://codereview.chromium.org/15966002/diff/9001/runtime/bin/bin.gypi#newco...
runtime/bin/bin.gypi:248: 'libdart_io',
Why was this needed?

https://codereview.chromium.org/15966002/diff/9001/runtime/bin/dartutils.cc
File runtime/bin/dartutils.cc (right):

https://codereview.chromium.org/15966002/diff/9001/runtime/bin/dartutils.cc#n...
runtime/bin/dartutils.cc:254: if (errno == ENOTCONN) {
Spinning is in general not a good idea. Also there is a Socket::SetBlocking
call, did that not work for you?

Is ENOTCONN supported on Windows?

[Cutch, 2013-05-24 17:59:56]

https://codereview.chromium.org/15966002/diff/9001/runtime/bin/bin.gypi
File runtime/bin/bin.gypi (right):

https://codereview.chromium.org/15966002/diff/9001/runtime/bin/bin.gypi#newco...
runtime/bin/bin.gypi:248: 'libdart_io',
On 2013/05/24 17:53:43, Ivan Posva wrote:
> Why was this needed?

Socket calls in libdart_builtin

https://codereview.chromium.org/15966002/diff/9001/runtime/bin/dartutils.cc
File runtime/bin/dartutils.cc (right):

https://codereview.chromium.org/15966002/diff/9001/runtime/bin/dartutils.cc#n...
runtime/bin/dartutils.cc:254: if (errno == ENOTCONN) {
On 2013/05/24 17:53:43, Ivan Posva wrote:
> Spinning is in general not a good idea. Also there is a Socket::SetBlocking
> call, did that not work for you?
> 

Socket::CreateConnect sets the socket to be non-blocking before the connect
call. Socket::SetNonBlocking takes effect after the connection. I do call
Socket::SetNonBlocking before the read. Alternatively, I can add a new
Socket::CreateConnectNonBlocking.

> Is ENOTCONN supported on Windows?

There is WSAENOTCONN and my impression from googling for this is that it is
supported. Need to verify that though.

[siva, 2013-05-24 23:23:18]

DBC.

https://codereview.chromium.org/15966002/diff/9001/runtime/bin/dartutils.cc
File runtime/bin/dartutils.cc (right):

https://codereview.chromium.org/15966002/diff/9001/runtime/bin/dartutils.cc#n...
runtime/bin/dartutils.cc:254: if (errno == ENOTCONN) {
I agree the notion of spinning seems contrived.

Maybe we should change the socket API to separate Create from
Connect. i.e have a Socket::Create(...) function and a
Socket::Connect(...) function. That way we will have the flexibility of making
the connect a blocking or non blocking operation.

On 2013/05/24 17:59:56, Cutch wrote:
> On 2013/05/24 17:53:43, Ivan Posva wrote:
> > Spinning is in general not a good idea. Also there is a Socket::SetBlocking
> > call, did that not work for you?
> > 
> 
> Socket::CreateConnect sets the socket to be non-blocking before the connect
> call. Socket::SetNonBlocking takes effect after the connection. I do call
> Socket::SetNonBlocking before the read. Alternatively, I can add a new
> Socket::CreateConnectNonBlocking.
> 
> > Is ENOTCONN supported on Windows?
> 
> There is WSAENOTCONN and my impression from googling for this is that it is
> supported. Need to verify that though.

https://codereview.chromium.org/15966002/diff/9001/runtime/bin/dartutils.cc#n...
runtime/bin/dartutils.cc:268: intptr_t buffer_size = 16 * 1024;
const intptr_t kbufferSize = 16 * KB;

to be uniform.

https://codereview.chromium.org/15966002/diff/9001/runtime/bin/dartutils.cc#n...
runtime/bin/dartutils.cc:276: }
This check for buffer overflow could be at the bottom of the do .. while loop
just after buffer_cursor += bytes_read;

https://codereview.chromium.org/15966002/diff/9001/runtime/bin/dartutils.cc#n...
runtime/bin/dartutils.cc:278: buffer_size-buffer_cursor-1);
We normally have a space between operators in our sources:
e.g:
buffer_size - buffer_cursor - 1

https://codereview.chromium.org/15966002/diff/9001/runtime/bin/dartutils.cc#n...
runtime/bin/dartutils.cc:287: buffer_cursor += bytes_read;
The increment could be after the if (bytes_read == 0) check.

https://codereview.chromium.org/15966002/diff/9001/runtime/bin/dartutils.cc#n...
runtime/bin/dartutils.cc:299: static const char* kOkayReply = "HTTP/1.0 200 OK";
Are we going with HTTP 1.0 and not 1.1

There seems to be some overlap in these messages with the stuff in
vmstats_impl.cc, maybe we could factor some of these out. Not for this CL but in
the future.

https://codereview.chromium.org/15966002/diff/9001/runtime/bin/dartutils.cc#n...
runtime/bin/dartutils.cc:313: // here. On the other hand the binary is about the
exit anyway.
Is this comment valid for this use case? The binary doesn't appear to be
exiting.

https://codereview.chromium.org/15966002/diff/9001/runtime/bin/dartutils.cc#n...
runtime/bin/dartutils.cc:386: Dart_Handle port = PortFromUri(uri, builtin_lib);
path, domain and port could be Error objects because Dart_Invoke(...) could have
returned Error.

https://codereview.chromium.org/15966002/diff/9001/runtime/bin/dartutils.cc#n...
runtime/bin/dartutils.cc:513: }
Seems like these three functions could have been factored into just one with an
additional 'const char*' parameter for the function name to invoke.

[Cutch, 2013-05-25 01:37:42]

https://codereview.chromium.org/15966002/diff/9001/runtime/bin/dartutils.cc
File runtime/bin/dartutils.cc (right):

https://codereview.chromium.org/15966002/diff/9001/runtime/bin/dartutils.cc#n...
runtime/bin/dartutils.cc:254: if (errno == ENOTCONN) {
On 2013/05/24 23:23:18, siva wrote:
> I agree the notion of spinning seems contrived.
> 
> Maybe we should change the socket API to separate Create from
> Connect. i.e have a Socket::Create(...) function and a
> Socket::Connect(...) function. That way we will have the flexibility of making
> the connect a blocking or non blocking operation.
> 
> On 2013/05/24 17:59:56, Cutch wrote:
> > On 2013/05/24 17:53:43, Ivan Posva wrote:
> > > Spinning is in general not a good idea. Also there is a
Socket::SetBlocking
> > > call, did that not work for you?
> > > 
> > 
> > Socket::CreateConnect sets the socket to be non-blocking before the connect
> > call. Socket::SetNonBlocking takes effect after the connection. I do call
> > Socket::SetNonBlocking before the read. Alternatively, I can add a new
> > Socket::CreateConnectNonBlocking.
> > 
> > > Is ENOTCONN supported on Windows?
> > 
> > There is WSAENOTCONN and my impression from googling for this is that it is
> > supported. Need to verify that though.
> 

Done.

https://codereview.chromium.org/15966002/diff/9001/runtime/bin/dartutils.cc#n...
runtime/bin/dartutils.cc:268: intptr_t buffer_size = 16 * 1024;
On 2013/05/24 23:23:18, siva wrote:
> const intptr_t kbufferSize = 16 * KB;
> 
> to be uniform.

Done.

https://codereview.chromium.org/15966002/diff/9001/runtime/bin/dartutils.cc#n...
runtime/bin/dartutils.cc:276: }
On 2013/05/24 23:23:18, siva wrote:
> This check for buffer overflow could be at the bottom of the do .. while loop
> just after buffer_cursor += bytes_read;

Done.

https://codereview.chromium.org/15966002/diff/9001/runtime/bin/dartutils.cc#n...
runtime/bin/dartutils.cc:278: buffer_size-buffer_cursor-1);
On 2013/05/24 23:23:18, siva wrote:
> We normally have a space between operators in our sources:
> e.g:
> buffer_size - buffer_cursor - 1

Done.

https://codereview.chromium.org/15966002/diff/9001/runtime/bin/dartutils.cc#n...
runtime/bin/dartutils.cc:287: buffer_cursor += bytes_read;
On 2013/05/24 23:23:18, siva wrote:
> The increment could be after the if (bytes_read == 0) check.

It can't be because I need to add the zero character to the correct position of
the buffer and set the response_length to the correct value.

https://codereview.chromium.org/15966002/diff/9001/runtime/bin/dartutils.cc#n...
runtime/bin/dartutils.cc:299: static const char* kOkayReply = "HTTP/1.0 200 OK";
On 2013/05/24 23:23:18, siva wrote:
> Are we going with HTTP 1.0 and not 1.1
> 

Yes, we don't need anything from 1.1.

> There seems to be some overlap in these messages with the stuff in
> vmstats_impl.cc, maybe we could factor some of these out. Not for this CL but
in
> the future.

Agreed that there is overlap with the vmstats code. However, I think the correct
long term strategy is to not implement our own HTTP client and server code and
adapt the Dart implementation to be easily called from C or use a small third
party library blessed by Google.

https://codereview.chromium.org/15966002/diff/9001/runtime/bin/dartutils.cc#n...
runtime/bin/dartutils.cc:313: // here. On the other hand the binary is about the
exit anyway.
On 2013/05/24 23:23:18, siva wrote:
> Is this comment valid for this use case? The binary doesn't appear to be
> exiting.

I believe what Ivan was saying is that the binary will be exiting almost
immediately after this happens (because the initial source or an imported source
file could not be loaded.).

https://codereview.chromium.org/15966002/diff/9001/runtime/bin/dartutils.cc#n...
runtime/bin/dartutils.cc:386: Dart_Handle port = PortFromUri(uri, builtin_lib);
On 2013/05/24 23:23:18, siva wrote:
> path, domain and port could be Error objects because Dart_Invoke(...) could
have
> returned Error.

Good catch. Done.

https://codereview.chromium.org/15966002/diff/9001/runtime/bin/dartutils.cc#n...
runtime/bin/dartutils.cc:513: }
On 2013/05/24 23:23:18, siva wrote:
> Seems like these three functions could have been factored into just one with
an
> additional 'const char*' parameter for the function name to invoke.

Done.

[Ivan Posva, 2013-05-28 18:25:32]

LGTM -ip

https://codereview.chromium.org/15966002/diff/25001/runtime/bin/dartutils.cc
File runtime/bin/dartutils.cc (right):

https://codereview.chromium.org/15966002/diff/25001/runtime/bin/dartutils.cc#...
runtime/bin/dartutils.cc:245: // Writes string into socket.
Please document the meaning of the return value.
Another possibility would be to just forward the error code instead of how far
you have written. Making it possible to print the error code in the error
message from where this is called.

https://codereview.chromium.org/15966002/diff/25001/runtime/bin/dartutils.cc#...
runtime/bin/dartutils.cc:327: -1,
Fits on one line?

[Cutch, 2013-05-28 21:39:55]

https://codereview.chromium.org/15966002/diff/25001/runtime/bin/dartutils.cc
File runtime/bin/dartutils.cc (right):

https://codereview.chromium.org/15966002/diff/25001/runtime/bin/dartutils.cc#...
runtime/bin/dartutils.cc:245: // Writes string into socket.
I return the error code and print it.

https://codereview.chromium.org/15966002/diff/25001/runtime/bin/dartutils.cc#...
runtime/bin/dartutils.cc:327: -1,
On 2013/05/28 18:25:32, Ivan Posva wrote:
> Fits on one line?

Done.

[Cutch, 2013-05-29 14:34:25]

Committed patchset #10 manually as r23351 (presubmit successful).