Fix panels not receiving latest messages on newly subscribed topic that is already subscribed to by another panel #7113

snosenzo · 2023-11-13T21:21:10Z

User-Facing Changes

Fix panels not receiving latest messages on newly subscribed topic that is already subscribed to by another panel

Description
Supercedes: #7090

Because "subscriptions" haven't changed according to the underlying player it does not re-emit state. Which means that updatePlayerStateAction on the message pipeline store is never called to set the lastMessages on the newly subscribed topics.

This PR moves the logic for applying lastMessages to subscribed topics to updateSubscriberAction to emit these messages immediately instead of waiting for another underlying player emit state. I've removed the need to store newTopics across the actions because handling these topics happens entirely in the updateSubscriberAction. I've also added some logic to remove messages on topics that are no longer subscribed to from lastMessageByTopic so that stale messages are not passed when resubscribing to that topic before backfill resolves.

Some notes about reading the PR:
I've removed some useDelayedFixture since they were causing strange behavior with some stories. These shouldn't be necessary anymore since the MockMessagePipelineProvider should now handle "Player" lifecycle and doing backfill for initial subscriptions better.

I had to add a pauseFrame functionality that more closely mirrored player listener behavior to the MMPP so that we wait until all components finish rendering before updating the store.

Fixes: FG-5574

…to topics

packages/studio-base/src/components/MessagePipeline/store.ts

respond to feedback

snosenzo · 2023-12-11T21:35:57Z

packages/studio-base/src/PanelAPI/useMessageReducer.ts


 type Params<T> = {
+  /**


commented this because I had to debug the tests and wanted to have a better understanding of the parameters

Not your problem in this PR to fix - but since useMessageReducer is an internal API I do wonder if it would be a "simple" PR to remove the string[] variant. Then you can also drop the sidecar preloadType field.

snosenzo · 2023-12-11T21:36:49Z

packages/studio-base/src/components/MessagePipeline/index.test.tsx

@@ -358,11 +358,7 @@ describe("MessagePipelineProvider/useMessagePipeline", () => {
    });
    expect(result.current.subscriptions).toEqual([{ topic: "/input/foo" }]);

-    // Emit empty player state to process new subscriptions


no longer need to wait for a player to emit again to receive the last messages on a topic

snosenzo · 2023-12-11T21:40:17Z

packages/studio-base/src/PanelAPI/useMessageReducer.test.tsx

    expect(result.current).toEqual(2);

    rerender({ topics: ["/bar", "/foo"] });

-    // no additional calls


no longer need player emit to push lastMessagesOnTOpic

packages/studio-base/src/components/MessagePipeline/MockMessagePipelineProvider.tsx

jtbandes · 2023-12-15T18:18:57Z

packages/studio-base/src/panels/ThreeDeeRender/stories/Urdfs.stories.tsx

-      activeData: {
-        currentTime: { sec: 0, nsec: 0 },
-        parameters: new Map([[urdfParamName, URDF3]]),
-        messages: [


was this just broken if these were getting ignored?

^ I too am curious - seems related to your Fixture code update about messages in activeData

These weren't being ignored before my change, but they weren't going through the normal message pipeline. They were emitted on the initialization of the store, but because there are no subscribers, nothing would handle them. They would not be emitted again because subsequent dispatching to the MP would overwrite it. putting them in the frame lets them go through the normal messages mock message pipeline prop

jtbandes · 2023-12-15T18:22:53Z

packages/studio-base/src/panels/ThreeDeeRender/stories/UrdfDisplayMode.stories.tsx

@@ -112,13 +111,11 @@ export const UrdfDisplayMode: StoryObj = {
      },
    };

-    const fixture = useDelayedFixture({


did having this break something?

This wasn't breaking anything as of my final implementation, but I believe it was responsible for the failing story in a previous iteration. Since it doesn't do anything anymore I'll keep it as is

defunctzombie · 2023-12-15T19:49:04Z

packages/studio-base/src/PanelAPI/useMessageReducer.ts


 type Params<T> = {
+  /**


Not your problem in this PR to fix - but since useMessageReducer is an internal API I do wonder if it would be a "simple" PR to remove the string[] variant. Then you can also drop the sidecar preloadType field.

defunctzombie · 2023-12-15T19:49:44Z

packages/studio-base/src/PanelAPI/useMessageReducer.ts

+   * @param state - Immutable. `undefined` when called on initialization or seek. Otherwise, the current state.
+   * @returns - New state. Must be new reference to trigger rerender.
+   */
+  restore: (state: T | undefined) => T;


The comment says state is immutable - what makes that so? Does it need to be changed to Immutable<T> ?

Was just trying to portray the original comment, will try to apply Immutable<T> but might complain in a bunch of places that I don't want to change in this PR

defunctzombie · 2023-12-15T19:50:20Z

packages/studio-base/src/PanelAPI/useMessageReducer.ts

@@ -30,18 +30,37 @@ import {

 const log = Log.getLogger(__filename);

-type MessageReducer<T> = (arg0: T, message: MessageEvent) => T;


fun fact - arg0 comes to us from the earliest days of when I converted the codebase from flow to typescript :D

defunctzombie · 2023-12-15T19:52:10Z

packages/studio-base/src/components/PanelExtensionAdapter/PanelExtensionAdapter.test.tsx

@@ -124,7 +124,7 @@ describe("PanelExtensionAdapter", () => {
      await Promise.resolve();
    });
    expect(renderStates).toEqual([
-      { currentFrame: [message], didSeek: false },
+      { currentFrame: [], didSeek: false }, // first frame is empty because there are no subscribers yet


If there are no subscribers yet why did the previous version of the test have messages in currentFrame?

because it would use the custom logic in MMPP that would add all of the existing messages to messagesBySubscriberId when update-subscriber was called. This now happens in a separate update-player action after update-subscribers was called.

defunctzombie · 2023-12-15T19:54:58Z

packages/studio-base/src/stories/PanelSetup.tsx

@@ -80,7 +80,10 @@ export type Fixture = {
  topics?: Topic[];
  capabilities?: string[];
  profile?: string;
-  activeData?: Partial<PlayerStateActiveData>;
+  /** Player active data does not include `messages`.


What does it mean that it does not include messages? Is it that any messages you provide in the activeData fixture are ignored?

defunctzombie · 2023-12-15T19:55:45Z

packages/studio-base/src/panels/ThreeDeeRender/stories/Urdfs.stories.tsx

-      activeData: {
-        currentTime: { sec: 0, nsec: 0 },
-        parameters: new Map([[urdfParamName, URDF3]]),
-        messages: [


^ I too am curious - seems related to your Fixture code update about messages in activeData

defunctzombie · 2023-12-15T19:56:26Z

packages/studio-base/src/components/MessagePipeline/index.test.tsx

@@ -581,11 +572,6 @@ describe("MessagePipelineProvider/useMessagePipeline", () => {
    });
    expect(result.current.subscriptions).toEqual([{ topic: "/input/foo" }]);

-    // Emit empty player state to process new subscriptions


👏 Always happy to see some doubleAct be deleted :D

defunctzombie · 2023-12-15T19:59:02Z

packages/studio-base/src/components/MessagePipeline/store.ts

@@ -62,7 +62,8 @@ export type MessagePipelineInternalState = {
   * for dispatching messages needs to iterate over the array of IDs.
   */
  subscriberIdsByTopic: Map<string, string[]>;
-  newTopicsBySubscriberId: Map<string, Set<string>>;
+  /** This holds the last message emitted by the player on each topic. Attempt to use this before falling back to player backfill.


Remind me what it means to fall-back to player backfill in this context?

player backfill reads the last message on all active subscriptions from the file and triggers a seek

defunctzombie · 2023-12-15T20:01:58Z

packages/studio-base/src/components/MessagePipeline/store.ts

@@ -227,36 +228,20 @@ function updateSubscriberAction(
  action: UpdateSubscriberAction,


Github is terrible at letting me comment outside of changed lines but the comment a few lines above this that mentions newTopicsBySubscriberId is wrong now. You can probably write a better comment on what this function does given the new implementation.

defunctzombie · 2023-12-15T20:03:07Z

packages/studio-base/src/components/MessagePipeline/store.ts

+    }
+  }
+
+  const messagesBySubscriberId = new Map<string, MessageEvent[]>();


Is SubscriberId the same ID as newTopicsForId a few lines up? Both are the subscriber id? How come we have a new map of messages by subscriberId here but the action only has one id?

If my reading of this is correct - the downsteam impact of this change is that whenever a panel subscribes, all panels will get notified with empty frame data? There is no carry-over of the messagesBySubscriberId from the existing state. Is that a problem? No test seemed to think it was a problem but it does seem like there's some gap here.

responded in slack, but useMessageReducer and PanelExtensionAdapter are memoized by the message array within messagesBySubscriberId and will not read in a new frame unless that array has changed

use message pipeline backfill for new panels with already subscribed-…

da396bf

…to topics

snosenzo requested review from jtbandes, defunctzombie and cfoust November 13, 2023 21:21

snosenzo mentioned this pull request Nov 13, 2023

Fix panels not receiving latest messages on newly subscribed topic that is already subscribed to by another panel. #7090

Closed

vercel bot deployed to Preview November 13, 2023 21:24 View deployment

defunctzombie reviewed Nov 14, 2023

View reviewed changes

packages/studio-base/src/components/MessagePipeline/store.ts Outdated Show resolved Hide resolved

renaming and commenting message reducer

c5ebc63

vercel bot deployed to Preview November 15, 2023 16:44 View deployment

snosenzo added 2 commits December 7, 2023 13:46

Merge branch 'main' into message-pipeline-backfill

c9c2652

update MockMessagePipelineProvider to fix tests

01eaf1f

respond to feedback

vercel bot deployed to Preview December 11, 2023 21:35 View deployment

snosenzo commented Dec 11, 2023

View reviewed changes

snosenzo marked this pull request as ready for review December 11, 2023 21:40

snosenzo added 2 commits December 12, 2023 16:14

Merge branch 'main' into message-pipeline-backfill

3f5d4a3

fix stories

dde4d0a

vercel bot deployed to Preview December 12, 2023 22:41 View deployment

hopefully fix stories

d0df140

vercel bot deployed to Preview December 14, 2023 15:43 View deployment

snosenzo commented Dec 14, 2023

View reviewed changes

packages/studio-base/src/components/MessagePipeline/MockMessagePipelineProvider.tsx Outdated Show resolved Hide resolved

fix stories?

b04136c

vercel bot deployed to Preview December 14, 2023 19:56 View deployment

only autoselect image should be failing now

de54165

vercel bot deployed to Preview December 15, 2023 02:40 View deployment

fix image only mode story

0cf434b

vercel bot deployed to Preview December 15, 2023 15:38 View deployment

jtbandes approved these changes Dec 15, 2023

View reviewed changes

defunctzombie reviewed Dec 15, 2023

View reviewed changes

snosenzo added 4 commits December 18, 2023 16:01

Merge branch 'main' into message-pipeline-backfill

3ecdd8e

Merge branch 'main' into message-pipeline-backfill

b947f51

update comments from feedback

ed9120c

update to only update new subscriber with messages

7a3a47c

vercel bot deployed to Preview December 19, 2023 16:42 View deployment

snosenzo merged commit 537f45b into main Dec 19, 2023
15 checks passed

snosenzo deleted the message-pipeline-backfill branch December 19, 2023 22:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix panels not receiving latest messages on newly subscribed topic that is already subscribed to by another panel #7113

Fix panels not receiving latest messages on newly subscribed topic that is already subscribed to by another panel #7113

snosenzo commented Nov 13, 2023 •

edited

Loading

snosenzo Dec 11, 2023

defunctzombie Dec 15, 2023

snosenzo Dec 11, 2023

snosenzo Dec 11, 2023

jtbandes Dec 15, 2023

defunctzombie Dec 15, 2023

snosenzo Dec 15, 2023 •

edited

Loading

jtbandes Dec 15, 2023

snosenzo Dec 15, 2023

defunctzombie Dec 15, 2023

defunctzombie Dec 15, 2023

snosenzo Dec 15, 2023

defunctzombie Dec 15, 2023

defunctzombie Dec 15, 2023

snosenzo Dec 18, 2023

defunctzombie Dec 15, 2023

defunctzombie Dec 15, 2023

defunctzombie Dec 15, 2023

defunctzombie Dec 15, 2023

snosenzo Dec 15, 2023

defunctzombie Dec 15, 2023

defunctzombie Dec 15, 2023

defunctzombie Dec 15, 2023

snosenzo Dec 19, 2023

		@@ -30,18 +30,37 @@ import {

		const log = Log.getLogger(__filename);

		type MessageReducer<T> = (arg0: T, message: MessageEvent) => T;

		@@ -227,36 +228,20 @@ function updateSubscriberAction(
		action: UpdateSubscriberAction,

Fix panels not receiving latest messages on newly subscribed topic that is already subscribed to by another panel #7113

Fix panels not receiving latest messages on newly subscribed topic that is already subscribed to by another panel #7113

Conversation

snosenzo commented Nov 13, 2023 • 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

snosenzo Dec 15, 2023 • 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

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

snosenzo commented Nov 13, 2023 •

edited

Loading

snosenzo Dec 15, 2023 •

edited

Loading