Use listener API for communication between build/server processes. #10077
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
benjamn
commented
Jul 16, 2018
| // package, one after the last. The first message waits for the | ||
| // package to call Package._define(packageName, exports). | ||
| promises[packageName] = ( | ||
| promises[packageName] || Package._promise(packageName) |
There was a problem hiding this comment.
Since we're no longer using Package._promise in core, and it was only recently introduced for this (now removed) code, I think we should remove it for the sake of keeping the APIs exposed by the core meteor package as small as possible.
benjamn
commented
Jul 16, 2018
| }); | ||
| // If arch is not a string, the receiver of this message should | ||
| // assume all clients need to be refreshed. | ||
| await appProcess.proc.sendMessage("client-refresh"); |
There was a problem hiding this comment.
If the autoupdate package is not installed, no message will be delivered to that package, so no error needs to be caught here.
benjamn
commented
Jul 16, 2018
| // autoupdate package. | ||
| Object.keys(cachesByPlatform).forEach(platform => { | ||
| delete cachesByPlatform[platform]; | ||
| }); |
There was a problem hiding this comment.
This is the part of the PR that actually fixes #10073.
Fixes #10073, per #10073 (comment) While thinking about this bug, I realized that sending IPC messages to specific packages in the server process was much less flexible than sending messages based on an arbitrary topic string, since the topic string approach allows both `autoupdate` and `dynamic-import` to listen for the same message. The topic string approach calls for a listener interface like `onMessage(topic, callback)`, which elegantly replaces the previous approach of requiring packages to export a single `onMessage` function. However, because the `meteor` package does not have access to the module system, implementing the `onMessage` listener interface in the `meteor` package would have required exposing an API like `Meteor.onMessage(topic, callback)`, which has an unpleasant global smell to it. Instead, the `onMessage` function should be explicitly imported (using the module system) from a less-generically-named package. Since I knew I was going to have to move the message dispatch logic out of the `meteor` package, I decided to create a new package called `inter-process-messaging` to implement the parent/child components of the IPC system.
benjamn
force-pushed
the
inter-process-messaging
branch
from
0c0a1e6
to
2450ffe
Compare
benjamn
commented
Jul 17, 2018
| const results = []; | ||
| const callbacks = callbacksByTopic.get(topic); | ||
| if (callbacks && callbacks.size > 0) { | ||
| callbacks.forEach(cb => results.push(cb(payload))); |
There was a problem hiding this comment.
One really nice benefit of using Set#forEach here is that if you add new elements to the set while you're traversing it, they will be included in the same forEach loop!
Every process is potentially the child of some other process and the parent of zero or more child processes of its own, so it's confusing to use terminology that always treats the current global.process as a "parent" process, or to include PARENT and CHILD in the message types. Instead, this new implementation uses message types MESSAGE, RESPONSE, PING, and PONG, and refers to `process` and `otherProcess` objects, with the caveat that sometimes `process === otherProcess`, because `process.send` can be used to send messages to the parent process. Instead of relying on the child to send a special CHILD_READY message to the parent when it's ready to receive messages, the sending process polls the receiving process with a preflight PING message, and the receiving process immediately responds with a PONG when ready.
|
I would like to extract this functionality as an npm package one day, but I think I need to draw the line on this endless refactoring somewhere. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #10073, per #10073 (comment)
While thinking about this bug, I realized that sending IPC messages to specific packages in the server process was much less flexible than sending messages based on an arbitrary topic string, since the topic string approach allows both
autoupdateanddynamic-importto listen for the same message.The topic string approach calls for a listener interface like
onMessage(topic, callback), which elegantly replaces the previous approach of requiring packages to export a singleonMessagefunction.However, because the
meteorpackage does not have access to the module system, implementing theonMessagelistener interface in themeteorpackage would have required exposing an API likeMeteor.onMessage(topic, callback), which has an unpleasant global smell to it. Instead, theonMessagefunction should be explicitly imported (using the module system) from some less-generically-named package.Since I knew I was going to have to move the message dispatch logic out of the
meteorpackage, I decided to create a new package calledinter-process-messagingto implement the parent/child components of the IPC system.