Add withAppEngineService() to package:appengine

Add withAppEngineService() to package:appengine

This will allow users to run background tasks which can use API services.

An example usage could look like:

  import 'dart:async';
  import 'dart:io';
  import 'package:appengine/appengine.dart';

  Future backgroundWork() {
    return dbService.query(Person).run().toList().then((List<Person> persons) {
      // Do something with `persons`.
    });
  }

  void mainHandler(HttpRequest request) {
    request.response..write('hello world')..close();
  }

  main() {
    withAppEngineServices(() {
      return Future.wait([
          runAppEngine(mainHandler),
          backgroundWork(),
      ]);
    });
  }

R=sgjesse@google.com, wibling@google.com

Committed: https://github.com/dart-lang/appengine/commit/1dd6742902a043fccbfe3a2cb62e9d6b38072cec

[kustermann, 2014-12-03 11:00:53]

kustermann@google.com changed reviewers:
+ sgjesse@google.com

[kustermann, 2014-12-03 11:01:05]

kustermann@google.com changed reviewers:
+ wibling@google.com

[wibling, 2014-12-03 11:11:03]

https://codereview.chromium.org/775043002/diff/20001/lib/src/appengine_intern...
File lib/src/appengine_internal.dart (right):

https://codereview.chromium.org/775043002/diff/20001/lib/src/appengine_intern...
lib/src/appengine_internal.dart:146: return
initializeAppEngine().then((ContextRegistry contextRegistry) {
Why call initializeAppEngine here? 

Isn't it enough to call withAppEngineServices() which then sets up the
_contextRegistry by calling initializeAppEngine?

[kustermann, 2014-12-03 11:14:42]

https://codereview.chromium.org/775043002/diff/20001/lib/src/appengine_intern...
File lib/src/appengine_internal.dart (right):

https://codereview.chromium.org/775043002/diff/20001/lib/src/appengine_intern...
lib/src/appengine_internal.dart:146: return
initializeAppEngine().then((ContextRegistry contextRegistry) {
On 2014/12/03 11:11:03, wibling wrote:
> Why call initializeAppEngine here? 
> 
> Isn't it enough to call withAppEngineServices() which then sets up the
> _contextRegistry by calling initializeAppEngine?

Sure. Forgot to remove that wrapper.

Done

[Søren Gjesse, 2014-12-03 11:17:42]

https://codereview.chromium.org/775043002/diff/20001/lib/appengine.dart
File lib/appengine.dart (right):

https://codereview.chromium.org/775043002/diff/20001/lib/appengine.dart#newco...
lib/appengine.dart:78: * scopes.
Please add the example from the code-review here.

Need some more information on the returned Future.

Can we add a future in the package which completes when shutting dowm. That way
we can suggest that that future is returned, e.g.

withAppEngineServices(() {
  // Startup stuff.

  // done is a global Future in the appengine package (should be called
something else).
  return done;
)}

This can also be done to run a "one shot" function after starting App Engine,
like this:

runAppEngine(..).then((_) {
  withAppEngineServices(() {
    // Do something, e.g. log startup info.
    return new Future.value();
  });
});

[kustermann, 2014-12-03 11:42:50]

Patchset #3 (id:40001) has been deleted

[kustermann, 2014-12-03 11:55:29]

PTAL

https://codereview.chromium.org/775043002/diff/20001/lib/appengine.dart
File lib/appengine.dart (right):

https://codereview.chromium.org/775043002/diff/20001/lib/appengine.dart#newco...
lib/appengine.dart:78: * scopes.
On 2014/12/03 11:17:42, Søren Gjesse wrote:
> Please add the example from the code-review here.
> 
> Need some more information on the returned Future.
> 
> Can we add a future in the package which completes when shutting dowm. That
way
> we can suggest that that future is returned, e.g.
> 
> withAppEngineServices(() {
>   // Startup stuff.
> 
>   // done is a global Future in the appengine package (should be called
> something else).
>   return done;
> )}
> 
> This can also be done to run a "one shot" function after starting App Engine,
> like this:
> 
> runAppEngine(..).then((_) {
>   withAppEngineServices(() {
>     // Do something, e.g. log startup info.
>     return new Future.value();
>   });
> });

Done -- added sample code.

Actually I think it's better if `runAppEngine()` just completes when we shut
down (which it actually does with this CL).

I like that much more. Then people can do cleanup when the request handling is
done, e.g. something like

main() {
  withAppEngineServics(() {
    return runAppEngine(handler).whenComplete(() {
      // Save global state, before shutting down VM.
      return dbService.commit(temporaryGlobalState);
    });
  });
}

[I think this is better than having a global done Future].


On the other hand it might be more complicated, if AppEngine will kill the VM
immediately after sending /_ah/stop, because then we would need to hold the
response back before completing. In which case we need something like:
registerShutdownHook() or something like that.

But this is orthogonal to the global done future vs. `runAppEngine()` Future.

[wibling, 2014-12-03 12:56:28]

lgtm

[Søren Gjesse, 2014-12-03 13:09:01]

lgtm

https://codereview.chromium.org/775043002/diff/20001/lib/appengine.dart
File lib/appengine.dart (right):

https://codereview.chromium.org/775043002/diff/20001/lib/appengine.dart#newco...
lib/appengine.dart:78: * scopes.
On 2014/12/03 11:55:29, kustermann wrote:
> On 2014/12/03 11:17:42, Søren Gjesse wrote:
> > Please add the example from the code-review here.
> > 
> > Need some more information on the returned Future.
> > 
> > Can we add a future in the package which completes when shutting dowm. That
> way
> > we can suggest that that future is returned, e.g.
> > 
> > withAppEngineServices(() {
> >   // Startup stuff.
> > 
> >   // done is a global Future in the appengine package (should be called
> > something else).
> >   return done;
> > )}
> > 
> > This can also be done to run a "one shot" function after starting App
Engine,
> > like this:
> > 
> > runAppEngine(..).then((_) {
> >   withAppEngineServices(() {
> >     // Do something, e.g. log startup info.
> >     return new Future.value();
> >   });
> > });
> 
> Done -- added sample code.
> 
> Actually I think it's better if `runAppEngine()` just completes when we shut
> down (which it actually does with this CL).
> 
> I like that much more. Then people can do cleanup when the request handling is
> done, e.g. something like
> 
> main() {
>   withAppEngineServics(() {
>     return runAppEngine(handler).whenComplete(() {
>       // Save global state, before shutting down VM.
>       return dbService.commit(temporaryGlobalState);
>     });
>   });
> }
> 
> [I think this is better than having a global done Future].
> 
> 
> On the other hand it might be more complicated, if AppEngine will kill the VM
> immediately after sending /_ah/stop, because then we would need to hold the
> response back before completing. In which case we need something like:
> registerShutdownHook() or something like that.
> 
> But this is orthogonal to the global done future vs. `runAppEngine()` Future.

In principle I am fine with this change to what the Future returned by
runAppEngine means. But it needs to be clearly documented. I think there will be
some time from /_ah/stop is called until the VM is killed. The documentation
says:

The stop lifecycle event is sent to your application when App Engine is
preparing to turn down the container. It is signaled as a HTTP request to the
handler /_ah/stop. The application does not need to respond to this event - but
can use it to perform any necessary clean up actions prior to the container
being shut down.

[kustermann, 2014-12-03 15:06:10]

On 2014/12/03 13:09:01, Søren Gjesse wrote:
> lgtm
> 
> https://codereview.chromium.org/775043002/diff/20001/lib/appengine.dart
> File lib/appengine.dart (right):
> 
>
https://codereview.chromium.org/775043002/diff/20001/lib/appengine.dart#newco...
> lib/appengine.dart:78: * scopes.
> On 2014/12/03 11:55:29, kustermann wrote:
> > On 2014/12/03 11:17:42, Søren Gjesse wrote:
> > > Please add the example from the code-review here.
> > > 
> > > Need some more information on the returned Future.
> > > 
> > > Can we add a future in the package which completes when shutting dowm.
That
> > way
> > > we can suggest that that future is returned, e.g.
> > > 
> > > withAppEngineServices(() {
> > >   // Startup stuff.
> > > 
> > >   // done is a global Future in the appengine package (should be called
> > > something else).
> > >   return done;
> > > )}
> > > 
> > > This can also be done to run a "one shot" function after starting App
> Engine,
> > > like this:
> > > 
> > > runAppEngine(..).then((_) {
> > >   withAppEngineServices(() {
> > >     // Do something, e.g. log startup info.
> > >     return new Future.value();
> > >   });
> > > });
> > 
> > Done -- added sample code.
> > 
> > Actually I think it's better if `runAppEngine()` just completes when we shut
> > down (which it actually does with this CL).
> > 
> > I like that much more. Then people can do cleanup when the request handling
is
> > done, e.g. something like
> > 
> > main() {
> >   withAppEngineServics(() {
> >     return runAppEngine(handler).whenComplete(() {
> >       // Save global state, before shutting down VM.
> >       return dbService.commit(temporaryGlobalState);
> >     });
> >   });
> > }
> > 
> > [I think this is better than having a global done Future].
> > 
> > 
> > On the other hand it might be more complicated, if AppEngine will kill the
VM
> > immediately after sending /_ah/stop, because then we would need to hold the
> > response back before completing. In which case we need something like:
> > registerShutdownHook() or something like that.
> > 
> > But this is orthogonal to the global done future vs. `runAppEngine()`
Future.
> 
> In principle I am fine with this change to what the Future returned by
> runAppEngine means. But it needs to be clearly documented. I think there will
be
> some time from /_ah/stop is called until the VM is killed. The documentation
> says:
> 
> The stop lifecycle event is sent to your application when App Engine is
> preparing to turn down the container. It is signaled as a HTTP request to the
> handler /_ah/stop. The application does not need to respond to this event -
but
> can use it to perform any necessary clean up actions prior to the container
> being shut down.

>  But it needs to be clearly documented.

I've added a doc comment to runAppEngine():
  46 *
  47 * The returned `Future` will complete when the HTTP server has been
shutdown
  48 * and is no longer serving requests.
  49 *

[kustermann, 2014-12-15 13:27:52]

Patchset #4 (id:80001) has been deleted

[kustermann, 2014-12-15 13:30:53]

Patchset #4 (id:100001) has been deleted

[kustermann, 2014-12-15 13:32:03]

I've added CHANGELOG.md/pubspec.yaml updates. If you're fine with this I'll
commit this and release this version?

[Søren Gjesse, 2014-12-15 14:18:15]

lgtm, go ahead and release this.

[kustermann, 2014-12-15 14:32:14]

Committed patchset #6 (id:160001) manually as
1dd6742902a043fccbfe3a2cb62e9d6b38072cec (presubmit successful).