feat: Enable isReady for debugger for GCF by gaofanmichael · Pull Request #358 · googleapis/cloud-debug-nodejs

gaofanmichael · 2017-11-06T19:46:09Z

This PR mainly serve for GCF. The introduced isReady will return a
promise, and to be resolved either

time since last breakpoint fetch is within a heuristic duration.
breakpoint update completed.

src/agent/debuglet.ts

+// to force the debug agent to return a new promise for isReady. The value is
+// the average of Stackdriver debugger hanging get duration (40s) and TCP
+// time-out on GCF (540s).
+const PROMISE_RESOLVE_CUT_OFF_IN_MILLISECONDS = 290 * 1000;


src/agent/debuglet.ts

+  // was successful. It is returned by isReady.
+  private breakpointFetched: Promise<void>|null;
+  // breakpointFetchedResolve is the resolve function for breakpointFetched.
+  private breakpointFetchedResolve: (() => void);


src/agent/debuglet.ts

+  // registration was successful.
+  private debuggeeRegistered: Promise<void>;
+  // debuggeeRegisteredResolve is the resolve function for debuggeeRegistered.
+  private debuggeeRegisteredResolve: (() => void);


src/agent/debuglet.ts

+        });
+        this.scheduleBreakpointFetch_(
+            0 /*immediately*/, true /*only fetch once*/);
+        return this.breakpointFetched;


src/index.ts


-  // We return the debuglet to facilitate testing.
-  return agentConfig.testMode_ ? debuglet : undefined;
+  return debuglet;


test/test-debuglet.ts

+      debugPromise
+          .then(() => {
+            setTimeout(function() {
+              debuglet.stop();


test/test-debuglet.ts

      debuglet.start();
    });

+    it('should resolve promises before exiting user functions', function(done) {


src/agent/debuglet.ts

+   * feature is mainly used in Google Cloud Function (GCF), as it is a
+   * serverless environment and we wanted to make sure debug agent always
+   * captures the snapshots.
+   */


src/agent/debuglet.ts

   * @private
   */
-  scheduleBreakpointFetch_(seconds: number): void {
+  scheduleBreakpointFetch_(seconds: number, onlyOnce: boolean): void {


src/agent/debuglet.ts

-                that.scheduleBreakpointFetch_(
-                    that.config.breakpointUpdateIntervalSec);
+                if (onlyOnce) {
+                  that.breakpointFetchedTimestamp = Date.now();


src/agent/debuglet.ts

+      if (this.breakpointFetched) {
+        return this.breakpointFetched;
+      }
+      this.debuggeeRegistered.then(() => {


kjin

LGTM from my end w/ nits.

src/agent/debuglet.ts

  private debuggeeRegistered: Promise<void>;
  // debuggeeRegisteredResolve is the resolve function for debuggeeRegistered.
-  private debuggeeRegisteredResolve: (() => void);
+  private debuggeeRegisteredResolve: () => void;


test/test-debuglet.ts

+                          .reply(200, {debuggee: {id: DEBUGGEE_ID}})
+                          .get(BPS_PATH + '?successOnTimeout=true')
+                          .reply(200, {breakpoints: [breakpoint]})
+                          .get(BPS_PATH + '?successOnTimeout=true')


src/agent/debuglet.ts

+  private promise: Promise<void>|null;
+  private promiseResolve: () => void;
+
+  constructor() {


src/agent/debuglet.ts

+            PROMISE_RESOLVE_CUT_OFF_IN_MILLISECONDS) {
+      return Promise.resolve();
+    } else {
+      if (this.breakpointFetched.get() !== null) {


test/test-debuglet.ts

+  it('CachedPromise.get() will be null if it has been cleared', (done) => {
+    this.timeout(2000);
+    let cachedPromise = new CachedPromise();
+    assert(cachedPromise.get() === null);


src/agent/debuglet.ts

+      this.promiseResolve = resolve;
+    });
+  }
+  get(): Promise<void>|null {


src/agent/debuglet.ts

+      this.breakpointFetched.refresh();
+      (this.debuggeeRegistered.get() as Promise<void>).then(() => {
+        this.scheduleBreakpointFetch_(
+          0 /*immediately*/, true /*only fetch once*/);


src/agent/debuglet.ts

+      if (this.breakpointFetched.get() !== null) {
+        return this.breakpointFetched.get() as Promise<void>;
+      }
+      this.breakpointFetched.refresh();


src/agent/debuglet.ts

+  private breakpointFetched: CachedPromise;
+  // debuggeeRegistered is a CachedPromise only to be resolved after debuggee
+  // registration was successful.
+  private debuggeeRegistered: CachedPromise;


gaofanmichael · 2017-11-10T01:57:01Z

@ofrobots @DominicKramer I have comments addressed. PTAL :)

src/agent/debuglet.ts

+/**
+ * IsReadyManager is a wrapper class to call debuglet.isReady().
+ */
+export class IsReadyManager {


src/agent/debuglet.ts

+    }
+  }
+  clear(): void {
+    this.promise = null;


src/agent/debuglet.ts

+/**
+ * IsReadyManager is a wrapper class to call debuglet.isReady().
+ */
+class IsReadyImpl implements IsReady {


src/agent/debuglet.ts

+        return result;
+      }
+      this.breakpointFetched.refresh();
+      (this.debuggeeRegistered.get() as Promise<void>).then(() => {


src/agent/debuglet.ts

                that.fetcherActive = false;
+                that.debuggeeRegistered.refresh();
+                that.breakpointFetched.resolve();
+                that.breakpointFetched.clear();


src/agent/debuglet.ts

              // breakpoint fetcher.
+              that.debuggeeRegistered.refresh();
+              that.breakpointFetched.resolve();
+              that.breakpointFetched.clear();


This PR mainly serve for GCF. The introduced isReady will return a promise, and to be resolved either 1) time since last breakpoint fetch is within a heuristic duration. 2) breakpoint update completed.

debuggee might expire, but debuggeRegistered promise is resolved. This was a bug introduced before. This commit fixed this by refreshing debuggeeRegistered promise when debuggee expired. Since debuggeeRegistered promise is been updated so the next time user calls isReady, a new debuggeeRegistered promise will return and user will wait upon that new promise for debuggee re-registration. Also, we will resolve breakpointFetch together so that no cloud function will hang there foreever.

src/agent/debuglet.ts

+ * function resolve() and can only be resolved once.
+ */
+export class CachedPromise {
+  private promiseResolve: (() => void) | null;


src/agent/debuglet.ts

+
+  get(): Promise<void> {
+    return this.promise;
+  }


src/agent/debuglet.ts

    this.completedBreakpointMap = {};
+
+    this.breakpointFetchedTimestamp = -Infinity;
+


src/agent/debuglet.ts

              // again after a while. Successful registration will restart the
              // breakpoint fetcher.
+              that.debuggeeRegistered = new CachedPromise();
+              if (that.breakpointFetched) {


src/agent/debuglet.ts

                that.logger.info('\t404 Registration expired.');
                that.fetcherActive = false;
+                that.debuggeeRegistered = new CachedPromise();
+                if (that.breakpointFetched) {


src/agent/debuglet.ts

+
+/**
+ * IsReady will return a promise to user after user starting the debug agent.
+ * This promise will be resolved when


src/agent/debuglet.ts

    this.completedBreakpointMap = {};
+
+    this.breakpointFetched = null;
+


src/agent/debuglet.ts

+    this.breakpointFetched = null;
+
+    this.breakpointFetchedTimestamp = -Infinity;
+


DominicKramer

LGTM after the tests pass

src/agent/debuglet.ts

+   * non-fetchable breakpoints.
+   */
+  updatePromise_() {
+    this.debuggeeRegistered = new CachedPromise();


gaofanmichael added the in progress label Nov 6, 2017

gaofanmichael requested review from DominicKramer, jinwoo, kjin and ofrobots November 6, 2017 19:46

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 6, 2017

jmdobry removed the cla: yes This human has signed the Contributor License Agreement. label Nov 6, 2017

gaofanmichael changed the title ~~chore: Enable isReady for debugger for GCF~~ feat: Enable isReady for debugger for GCF Nov 6, 2017

DominicKramer suggested changes Nov 7, 2017

View reviewed changes

gaofanmichael mentioned this pull request Nov 7, 2017

Support GCF through a 'initialization completed promise' #341

Closed

kjin reviewed Nov 7, 2017

View reviewed changes

gaofanmichael force-pushed the gcf2 branch from 89cc63d to b62dc6c Compare November 7, 2017 23:58

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 7, 2017

gaofanmichael force-pushed the gcf2 branch from b62dc6c to 2624a5b Compare November 8, 2017 00:30

kjin approved these changes Nov 8, 2017

View reviewed changes

gaofanmichael force-pushed the gcf2 branch 2 times, most recently from 036f402 to 1cbb8d3 Compare November 8, 2017 23:16

DominicKramer reviewed Nov 9, 2017

View reviewed changes

src/agent/debuglet.ts Outdated

private promise: Promise<void>|null;

private promiseResolve: () => void;

constructor() {

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

DominicKramer reviewed Nov 9, 2017

View reviewed changes

src/agent/debuglet.ts Outdated

PROMISE_RESOLVE_CUT_OFF_IN_MILLISECONDS) {

return Promise.resolve();

} else {

if (this.breakpointFetched.get() !== null) {

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

DominicKramer reviewed Nov 9, 2017

View reviewed changes

kjin reviewed Nov 9, 2017

View reviewed changes

src/agent/debuglet.ts Outdated

this.promiseResolve = resolve;

});

}

get(): Promise<void>|null {

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

ofrobots reviewed Nov 9, 2017

View reviewed changes

src/agent/debuglet.ts

this.breakpointFetched.refresh();

(this.debuggeeRegistered.get() as Promise<void>).then(() => {

this.scheduleBreakpointFetch_(

0 /*immediately*/, true /*only fetch once*/);

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

ofrobots reviewed Nov 9, 2017

View reviewed changes

src/agent/debuglet.ts Outdated

if (this.breakpointFetched.get() !== null) {

return this.breakpointFetched.get() as Promise<void>;

}

this.breakpointFetched.refresh();

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

ofrobots reviewed Nov 9, 2017

View reviewed changes

gaofanmichael force-pushed the gcf2 branch from 89e82e1 to c58c8c4 Compare November 10, 2017 01:26

DominicKramer reviewed Nov 10, 2017

View reviewed changes

src/agent/debuglet.ts Outdated

/**

* IsReadyManager is a wrapper class to call debuglet.isReady().

*/

export class IsReadyManager {

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

DominicKramer reviewed Nov 14, 2017

View reviewed changes

gaofanmichael force-pushed the gcf2 branch 2 times, most recently from 6312c8a to ee39261 Compare November 15, 2017 19:23

gaofanmichael force-pushed the gcf2 branch from ee39261 to 9c4c6d1 Compare November 15, 2017 21:48

gaofanmichael added 7 commits November 15, 2017 14:51

chore: Enable isReady for debugger for GCF

3838987

This PR mainly serve for GCF. The introduced isReady will return a promise, and to be resolved either 1) time since last breakpoint fetch is within a heuristic duration. 2) breakpoint update completed.

Address comments

ee48a0b

Abstract to CachedPromise

7d97839

Add opaque object with isReady

cc16f0c

add interface

57855b3

simplify CachedPromise

a55a816

gaofanmichael force-pushed the gcf2 branch from 9c4c6d1 to a55a816 Compare November 15, 2017 23:05

DominicKramer reviewed Nov 16, 2017

View reviewed changes

gaofanmichael added 2 commits November 16, 2017 11:24

refactor

dbf0a02

add doc for IsReady

73ebadf

DominicKramer reviewed Nov 16, 2017

View reviewed changes

src/agent/debuglet.ts Outdated

/**

* IsReady will return a promise to user after user starting the debug agent.

* This promise will be resolved when

This comment was marked as spam.

Sign in to view

DominicKramer reviewed Nov 16, 2017

View reviewed changes

src/agent/debuglet.ts Outdated

this.completedBreakpointMap = {};

this.breakpointFetched = null;

This comment was marked as spam.

Sign in to view

DominicKramer reviewed Nov 16, 2017

View reviewed changes

src/agent/debuglet.ts Outdated

this.breakpointFetched = null;

this.breakpointFetchedTimestamp = -Infinity;

This comment was marked as spam.

Sign in to view

DominicKramer approved these changes Nov 16, 2017

View reviewed changes

ofrobots reviewed Nov 16, 2017

View reviewed changes

src/agent/debuglet.ts

* non-fetchable breakpoints.

*/

updatePromise_() {

this.debuggeeRegistered = new CachedPromise();

This comment was marked as spam.

Sign in to view

address comments

ebbc252

gaofanmichael force-pushed the gcf2 branch from 8ce504a to ebbc252 Compare November 16, 2017 22:47

gaofanmichael merged commit a828ea6 into googleapis:master Nov 16, 2017

jmdobry removed the in progress label Nov 16, 2017

gaofanmichael deleted the gcf2 branch November 27, 2017 21:23

		this.completedBreakpointMap = {};

		this.breakpointFetchedTimestamp = -Infinity;

		this.completedBreakpointMap = {};

		this.breakpointFetched = null;

Conversation

gaofanmichael commented Nov 6, 2017

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

kjin left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!