Skip to content

Conversation

@sirtimid
Copy link
Contributor

@sirtimid sirtimid commented Nov 4, 2024

Closes #214

This PR resolves an issue with multiple ChromeRuntimeDuplexStream instances using the same endpoint (e.g., offscreen), in our case when there were multiple connections involve offscreen, such as between background and offscreen and also devtools and offscreen.

For example, when devtools and background both have offscreen as a target, there was no way to tell where the messages were coming from, leading to unexpected errors. By adding a source filter, we can now differentiate the messages, so each connection is handled correctly.

Changes:

  • Introduced a filter in ChromeRuntimeReader based on the source of incoming messages. This allows us to differentiate between connections and prevent unexpected message errors.
  • Updated ChromeRuntimeWriter to include the source in each message, ensuring that messages are directed to the correct target in multi-connection setups.

@sirtimid sirtimid requested a review from a team as a code owner November 4, 2024 20:20
@sirtimid sirtimid requested a review from rekmarks November 4, 2024 20:25
Comment on lines 109 to 116
if (message.target !== this.#target || message.source !== this.#source) {
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think returning is acceptable behavior but we should still console.info for the time being. See this comment for a prior discussion suggesting a convention for logger methods. This will result in logger output under regular use, but I think we would rather taper that off via log levels than not have the feedback at all.

Also, since this PR specifies the Reader and Writer to a specific directed edge of our comms network, we should consider using the local and remote nomenclature here to specify the edge, instead of target which specifies either the local or remote node depending on whether the stream is for reading or writing.

Suggested change
if (message.target !== this.#target || message.source !== this.#source) {
return;
}
if (message.target !== this.#local || message.source !== this.#remote) {
console.info(
`ChromeRuntimeReader received message for unexpected target: ${stringify(
message,
)}`,
);
return;
}

The local/remote names mirror what is done with the BaseDuplexStream.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logging there will be misleading imo since runtime.onMessage listener essentially catches all messages, but I'm ok if we use console.debug so they appear in verbose mode.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reading this reference on log levels I agree with your assessment that debug is the right method here. I think we would benefit from a convention for when to use each of these methods. I added a discussion issue for the topic.

Copy link
Contributor

@grypez grypez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get why we need to do this. Although the messages use source and target properties, I think the newly specified Reader and Writer should follow the local / remote convention established in the DuplexStream.

@sirtimid
Copy link
Contributor Author

sirtimid commented Nov 5, 2024

I get why we need to do this. Although the messages use source and target properties, I think the newly specified Reader and Writer should follow the local / remote convention established in the DuplexStream.

This is not exactly the case and it can lead to confusion, since, for ChromeRuntimeReader the target is the localTarget but for ChromeRuntimeWriter the target is the remoteTarget of the ChromeRuntimeDuplexStream. I think what we have now is clearer.

Copy link
Contributor

@grypez grypez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for ChromeRuntimeReader the target is the localTarget but for ChromeRuntimeWriter the target is the remoteTarget of the ChromeRuntimeDuplexStream. I think what we have now is clearer.

Well, the difference is that with the source/target nomenclature the user of Reader and Writer should permute source and target at construction. That is, when setting up a Reader and Writer at node A to talk to node B, the Reader should have target A and source B while the Writer should have target B and source A. With the local and remote nomenclature, the user passes local A and remote B to both the Reader and Writer.

Before this PR we were implicitly naming directed edges of the network based on assumptions about Reader and Writer. After this PR we will be naming the directed edges explicitly by both the head and tail nodes, but which node is the head and which is the tail depends on whether we are using a Reader or Writer. I think since we will never be able to instantiate A's reader and B's writer in the same realm, we are better served aligning the arguments for the A --> B writer with the B --> A reader, i.e. using the local/remote convention.

But this is all merely a convention and the code works. I don't think I can make any deeper argument for my view so I'll approve. Thanks for reading!

@rekmarks rekmarks merged commit a79cdb2 into main Nov 6, 2024
@rekmarks rekmarks deleted the sirtimid/chrome-duplex-stream branch November 6, 2024 07:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: ChromeRuntimeDuplexStream fails when multiple streams share the same target

4 participants