Makes viewer.sendMessageUnreliable reliably return promise of response. #6191

lannka · 2016-11-15T00:52:50Z

as part of #6159

lannka · 2016-11-15T00:54:52Z

src/service/viewer-impl.js

@@ -1005,7 +1005,7 @@ export class Viewer {
      return Promise.reject(getChannelError());
    }
    return this.messagingReadyPromise_.then(() => {
-      return this.sendMessageUnreliable_(eventType, data, awaitResponse);
+      return this.messageDeliverer_(eventType, data, awaitResponse);


not sure why original code calls sendMessageUnreliable_.

Basically, the message is NOT guaranteed to be delivered. It doesn't wait for the messaging to be established and may or may not be queued.

It's in the messagingReadyPromise, so it will never be queued. it's exactly the same to call either sendMessageUnreliable_ or messageDeliverer_ here.

It reduces duplication, so that only #sendMessageUnreliable knows how to really send a message. This should continue to call the unreliable method (since we've assured it's now "reliable" by waiting for messaging to be established).

@jridgewell don't quite understand what duplication we reduced here.

Imagine we add a 4th param to messageDeliverer function. Now we have to add that to 3 spots instead of just #sendMessageUnreliable.

Got you. but logically it doesn't make too much sense because the messaging is there, and we know we can send it "reliablely", why still call "sendUnreliable". Anyway, I don't want to be off the topic here too much since we're actually thinking of comletely removing sendMessageUnreliable_

The naming is unimportant (this could be called #postMessage and #reliablyPostMessage). The benefit is the de-duplication.

I disagree. It doesn't make sense to call

function sendMessage(message) { if (messagingReady) { sendMessageCancelUnsent(message); } }

lannka · 2016-11-15T00:55:03Z

src/service/viewer-impl.js

@@ -1078,7 +1077,7 @@ export class Viewer {
    }
    this.messagingMaybePromise_.then(() => {
      if (this.messageDeliverer_) {
-        this.sendMessageUnreliable_(eventType, data, false);
+        this.messageDeliverer_(eventType, data, false);


not sure why original code calls sendMessageUnreliable_.

We should have just checked for #messagingMaybePromise, then called #sendMessage.

dvoytenko · 2016-11-15T17:35:42Z

src/service/viewer-impl.js

@@ -742,7 +742,7 @@ export class Viewer {
   * Triggers "documentLoaded" event for the viewer.
   */
  postDocumentReady() {
-    this.sendMessageUnreliable_('documentLoaded', {
+    this.sendMessageUnreliable('documentLoaded', {


I'd like to reconsider these calls in an alternative way. We already have a very clear idea whether or not (a) doc is embedded; and (b) where the messaging is expected. With this in mind, I'd rather change this code to either wait for messaging ready and reliably send this message or not even try if messaging is not coming. We currently have, I believe, maybeSendMessage_ that does something like that in Viewer.

Some code in "sendMessageUnreliable" has a useful feature to de-dupe the messages. But it's too coarse and doesn't work very well for all cases. I'd rather move deduplication, if needed, to the actual class that issues messages.

WDYT? So, the bottom line, the sendMessageUnreliable_ will be completely gone.

I agree with you that the purpose of sendMessageUnreliable is a bit unclear. If you think we can live without the the de-dup logic, then this can be completely gone.

Also want to blame the method for its returned promise, which is indeterministic, caller should not actually rely on it.

I'll change all current usage of sendMessageUnreliable just to sendMessage if it sounds good to you.

After discussion, de-dup logic is actually hard to implement outside this class.

I made change to sendMessageUnreliable so that it will have a consistent return value. And I renamed it to sendMessageCancelUnsent. Let me know if this makes sense.

…named it to sendMessageCancelUnsent

lannka · 2016-11-16T04:49:03Z

@dvoytenko PTAL

lannka · 2016-11-16T23:56:18Z

TODO added. @dvoytenko PTAL

muxin · 2016-11-17T00:00:50Z

src/service/viewer-impl.js

@@ -834,6 +841,7 @@ export class Viewer {
   * Get the fragment from the url or the viewer.
   * Strip leading '#' in the fragment
   * @return {!Promise<string>}
+   * TODO: move this to amp-share-tracking


This method might need to stay inside viewer-impl

It's only used by amp-share-tracking. Not sure if anyone else is interested. If later it's really sharable code, we should probably extract it to somewhere else.

I think we decided to move this to history service?

probably not a bad idea

yeah, should be in history-impl

muxin · 2016-11-17T00:02:30Z

src/service/viewer-impl.js

@@ -863,6 +871,7 @@ export class Viewer {
   * The fragment variable should contain leading '#'
   * @param {string} fragment
   * @return {!Promise}
+   * TODO: move this to amp-share-tracking, and use sendMessage()


This method should also stay

Likewise would go to history?

history-impl sounds good.

dvoytenko · 2016-11-17T00:39:15Z

src/utils/array.js

+ */
+export function findIndex(array, predicate) {
+  let i = 0;
+  while (i < array.length) {


Do we expect predicate to mutate array? Why not use for(let i = 0; ...)?

predicate should not mutate array.
for and while are the same here. unless you insist :-)

👍 for.

OK, I surrender.

dvoytenko · 2016-11-17T00:41:07Z

src/service/viewer-impl.js

@@ -834,6 +841,7 @@ export class Viewer {
   * Get the fragment from the url or the viewer.
   * Strip leading '#' in the fragment
   * @return {!Promise<string>}
+   * TODO: move this to amp-share-tracking


I think we decided to move this to history service?

dvoytenko · 2016-11-17T00:41:49Z

src/service/viewer-impl.js

@@ -844,7 +853,7 @@ export class Viewer {
    if (!this.hasCapability('fragment')) {
      return Promise.resolve('');
    }
-    return this.sendMessageUnreliable_('fragment', undefined, true).then(
+    return this.sendMessageCancelUnsent('fragment', undefined, true).then(


This should be switched to getMessage right away. "cancel unsent" really wrong here.

What can go wrong? It's still a tiny optimization, no?
Considering multiple getFragment() were queued up, and when messaging channel established, we can just get it once from Viewer and respond to all interested party.

There will be only one getFragment call.
Without the modified queued up change in this PR, the previous sendMessageUnreliable_ is wrong, since it will get a resolved promise without getting the fragment if the message is sent before message channel is established. But now I think we should use sendMessageCancelUnsent because it now only resolves when the message channel is established and get a response from the viewer, which is exactly what we want.

dvoytenko · 2016-11-17T00:42:08Z

src/service/viewer-impl.js

@@ -863,6 +871,7 @@ export class Viewer {
   * The fragment variable should contain leading '#'
   * @param {string} fragment
   * @return {!Promise}
+   * TODO: move this to amp-share-tracking, and use sendMessage()


Likewise would go to history?

dvoytenko · 2016-11-17T00:43:56Z

src/service/viewer-impl.js

   */
  tick(message) {
-    this.sendMessageUnreliable_('tick', message, false);
+    this.sendMessageCancelUnsent('tick', message, false);


This one is tricky :( In case of ticks, the first tick is the most important. How can we approach this?

yep, so the TODO is to make it sendMessage(). i think we should not cancel any unsent ones.

dvoytenko · 2016-11-17T00:44:31Z

src/service/viewer-impl.js

  }

  /**
   * Triggers "prerenderComplete" event for the viewer.
   * @param {!Object} message
   */
  prerenderComplete(message) {
-    this.sendMessageUnreliable_('prerenderComplete', message, false);
+    this.sendMessageCancelUnsent('prerenderComplete', message, false);


TODO to be moved to either Resources or Performance.

performance

dvoytenko · 2016-11-17T00:48:02Z

src/service/viewer-impl.js

+    if (found != -1) {
+      message = this.messageQueue_.splice(found, 1)[0];
+      message.data = data;
+      message.awaitResponse = awaitResponse;


It should probably be message.awaitResponse |= awaitResponse?

dvoytenko · 2016-11-17T00:48:33Z

src/service/viewer-impl.js

-      // TODO(dvoytenko): This is somewhat questionable. What do we return
-      // when no one is listening?
-      return Promise.resolve();
+      let responseResolver;


Do we always need to create it? Or only the first time we run into awaitResponse = true?

logically, yes, no need. but for code simplicity, i think it's harmless?

dvoytenko · 2016-11-17T00:50:04Z

src/service/viewer-impl.js

+      const responsePromise = new Promise(r => {
+        responseResolver = r;
+      });
+      message = {


Not seeing the typedef for the messageQueue_ updated anywhere...

lannka

comments addressed

lannka · 2016-11-17T00:51:42Z

src/service/viewer-impl.js

@@ -834,6 +841,7 @@ export class Viewer {
   * Get the fragment from the url or the viewer.
   * Strip leading '#' in the fragment
   * @return {!Promise<string>}
+   * TODO: move this to amp-share-tracking


probably not a bad idea

lannka · 2016-11-17T00:54:02Z

src/service/viewer-impl.js

@@ -844,7 +853,7 @@ export class Viewer {
    if (!this.hasCapability('fragment')) {
      return Promise.resolve('');
    }
-    return this.sendMessageUnreliable_('fragment', undefined, true).then(
+    return this.sendMessageCancelUnsent('fragment', undefined, true).then(


What can go wrong? It's still a tiny optimization, no?
Considering multiple getFragment() were queued up, and when messaging channel established, we can just get it once from Viewer and respond to all interested party.

lannka · 2016-11-17T00:54:22Z

src/service/viewer-impl.js

@@ -863,6 +871,7 @@ export class Viewer {
   * The fragment variable should contain leading '#'
   * @param {string} fragment
   * @return {!Promise}
+   * TODO: move this to amp-share-tracking, and use sendMessage()


history-impl sounds good.

lannka · 2016-11-17T00:55:00Z

src/service/viewer-impl.js

   */
  tick(message) {
-    this.sendMessageUnreliable_('tick', message, false);
+    this.sendMessageCancelUnsent('tick', message, false);


yep, so the TODO is to make it sendMessage(). i think we should not cancel any unsent ones.

lannka · 2016-11-17T00:56:02Z

src/service/viewer-impl.js

+    if (found != -1) {
+      message = this.messageQueue_.splice(found, 1)[0];
+      message.data = data;
+      message.awaitResponse = awaitResponse;


lannka · 2016-11-17T00:57:50Z

src/service/viewer-impl.js

-      // TODO(dvoytenko): This is somewhat questionable. What do we return
-      // when no one is listening?
-      return Promise.resolve();
+      let responseResolver;


logically, yes, no need. but for code simplicity, i think it's harmless?

lannka · 2016-11-17T00:59:01Z

src/utils/array.js

+ */
+export function findIndex(array, predicate) {
+  let i = 0;
+  while (i < array.length) {


predicate should not mutate array.
for and while are the same here. unless you insist :-)

lannka · 2016-11-17T01:01:12Z

src/service/viewer-impl.js

  }

  /**
   * Triggers "prerenderComplete" event for the viewer.
   * @param {!Object} message
   */
  prerenderComplete(message) {
-    this.sendMessageUnreliable_('prerenderComplete', message, false);
+    this.sendMessageCancelUnsent('prerenderComplete', message, false);


performance

jridgewell · 2016-11-17T17:58:29Z

src/service/viewer-impl.js

+            message.eventType, message.data, message.awaitResponse);
+
+        if (message.awaitResponse) {
+          responsePromise.then(response => message.responseResolver(response));


You can optimize this by resolving with the promise: message.responseResolver(responsePromise)

jridgewell · 2016-11-17T17:59:45Z

src/service/viewer-impl.js

@@ -1005,7 +1005,7 @@ export class Viewer {
      return Promise.reject(getChannelError());
    }
    return this.messagingReadyPromise_.then(() => {
-      return this.sendMessageUnreliable_(eventType, data, awaitResponse);
+      return this.messageDeliverer_(eventType, data, awaitResponse);


The naming is unimportant (this could be called #postMessage and #reliablyPostMessage). The benefit is the de-duplication.

jridgewell · 2016-11-17T18:02:15Z

src/service/viewer-impl.js

   */
-  sendMessageUnreliable_(eventType, data, awaitResponse) {
+  sendMessageCancelUnsent(eventType, data, awaitResponse) {


I don't see why we care about awaitResponse at all. We always create the promise, why not just return it and let the caller decide if it cares (by calling #then).

it's just how the original messageDeliverer function is defined, which can return undefined. It doesn't really matter.

I see one little advantage of returning undefined is that it will throw immediately if you made mistake of awaitResponse=false when it should be true.

Why would you want that, though? Why declare my intention twice, with both a boolean and my interacting with the return value (the promise is always generated).

fine. but this is how the external messageDeliverer is defined. it's not a bad idea to keep it the same. let's just keep it as is.

jridgewell · 2016-11-17T18:03:32Z

src/utils/array.js

+ */
+export function findIndex(array, predicate) {
+  let i = 0;
+  while (i < array.length) {


👍 for.

jridgewell · 2016-11-17T18:04:03Z

src/utils/array.js

+    }
+    i++;
+  }
+  return i < array.length ? i : -1;


You can skip this awkwardness by returning in the loop.

dvoytenko · 2016-11-17T18:41:57Z

LGTM on my side, pending the comments from others.

lannka

All comments addressed.

lannka · 2016-11-17T21:37:39Z

src/service/viewer-impl.js

@@ -1005,7 +1005,7 @@ export class Viewer {
      return Promise.reject(getChannelError());
    }
    return this.messagingReadyPromise_.then(() => {
-      return this.sendMessageUnreliable_(eventType, data, awaitResponse);
+      return this.messageDeliverer_(eventType, data, awaitResponse);


I disagree. It doesn't make sense to call

function sendMessage(message) { if (messagingReady) { sendMessageCancelUnsent(message); } }

lannka · 2016-11-17T21:42:24Z

src/service/viewer-impl.js

   */
-  sendMessageUnreliable_(eventType, data, awaitResponse) {
+  sendMessageCancelUnsent(eventType, data, awaitResponse) {


it's just how the original messageDeliverer function is defined, which can return undefined. It doesn't really matter.

I see one little advantage of returning undefined is that it will throw immediately if you made mistake of awaitResponse=false when it should be true.

lannka · 2016-11-17T21:44:21Z

src/service/viewer-impl.js

+      const responsePromise = new Promise(r => {
+        responseResolver = r;
+      });
+      message = {


lannka · 2016-11-17T21:45:24Z

src/utils/array.js

+ */
+export function findIndex(array, predicate) {
+  let i = 0;
+  while (i < array.length) {


OK, I surrender.

lannka · 2016-11-17T21:45:31Z

src/utils/array.js

+    }
+    i++;
+  }
+  return i < array.length ? i : -1;


jridgewell · 2016-11-17T22:29:25Z

src/service/viewer-impl.js

+
+    let message;
+    if (found != -1) {
+      message = this.messageQueue_.splice(found, 1)[0];


Why splice it out? We can just move the push into the else condition.

I don't get it. we want to cancel the found message

How have we canceled it? We're just pushing the exact same object (with modifications) back on to the queue.

Oh, because we want the new message to be at the tail. Order matters here, think about message like flushTick.

Hang on. We are not cancelling the older message are we? We are resolving them all, right?

lannka · 2016-11-17T23:53:23Z

Thanks! merging...

…e. (ampproject#6191) * Makes viewer.sendMessageUnreliable public. * Make sendMessageUnreliable to return a real response promise. Also renamed it to sendMessageCancelUnsent * Make sure new message is added to the tail. * Add tests. * nit * Add TODOs. * Address comments. * Address comments

Makes viewer.sendMessageUnreliable public.

3739e72

lannka added the NEEDS REVIEW label Nov 15, 2016

lannka assigned dvoytenko and muxin Nov 15, 2016

lannka commented Nov 15, 2016

View reviewed changes

dvoytenko reviewed Nov 15, 2016

View reviewed changes

Make sendMessageUnreliable to return a real response promise. Also re…

5782800

…named it to sendMessageCancelUnsent

lannka changed the title ~~Makes viewer.sendMessageUnreliable public.~~ Makes viewer.sendMessageUnreliable return promise of response. Nov 15, 2016

lannka added 2 commits November 15, 2016 16:58

Make sure new message is added to the tail.

367484f

Add tests.

26cbf00

lannka changed the title ~~Makes viewer.sendMessageUnreliable return promise of response.~~ Makes viewer.sendMessageUnreliable reliably return promise of response. Nov 16, 2016

lannka added 2 commits November 16, 2016 08:33

nit

0be8b69

Add TODOs.

cb77713

muxin reviewed Nov 17, 2016

View reviewed changes

dvoytenko suggested changes Nov 17, 2016

View reviewed changes

Address comments.

cf551ed

lannka commented Nov 17, 2016

View reviewed changes

jridgewell reviewed Nov 17, 2016

View reviewed changes

dvoytenko approved these changes Nov 17, 2016

View reviewed changes

Address comments

1b535bb

lannka commented Nov 17, 2016

View reviewed changes

jridgewell reviewed Nov 17, 2016

View reviewed changes

jridgewell approved these changes Nov 17, 2016

View reviewed changes

lannka removed the NEEDS REVIEW label Nov 17, 2016

lannka merged commit 68802ad into ampproject:master Nov 17, 2016

lannka deleted the viewer_send_message_public branch November 17, 2016 23:53

jridgewell mentioned this pull request Nov 21, 2016

Refactor methods in viewer into performance-impl #6252

Merged

ampprojectbot mentioned this pull request Jan 10, 2022

🚨 Error: Cannot read properties of null (reading 'setState') ampproject/error-reporting#121

Open

Makes viewer.sendMessageUnreliable reliably return promise of response. #6191

Makes viewer.sendMessageUnreliable reliably return promise of response. #6191

Conversation

lannka commented Nov 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lannka Nov 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lannka commented Nov 16, 2016

lannka commented Nov 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lannka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvoytenko commented Nov 17, 2016

lannka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lannka Nov 15, 2016 •

edited

Loading