-
Notifications
You must be signed in to change notification settings - Fork 514
fix: INTERPRETING.md broadcast and receiveBroadcast function documentation is out of date #4693
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix: INTERPRETING.md broadcast and receiveBroadcast function documentation is out of date #4693
Conversation
…ation is out of date
gibson042
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dispute that the broadcast/receiveBroadcast API requirements currently described in INTERPRETING.md are out of date. We could move to the simplification proposed here with no impact to current test files, but I'm not confident that future testing wouldn't benefit from the second parameter. Is this being motivated by an implementation burden?
| - **`receiveBroadcast`** - a function that takes a function and | ||
| calls the function when it has received a broadcast from the parent, | ||
| passing it the broadcast as two arguments, a SharedArrayBuffer and | ||
| an Int32 or BigInt. This function may return before a broadcast is received | ||
| (eg to return to an event loop to await a message) and no code should | ||
| follow the call to this function. | ||
| - **`receiveBroadcast`** - a function that takes a callback and | ||
| calls the callback when it has received a broadcast from the parent, | ||
| passing it the broadcast as a single SharedArrayBuffer. This function | ||
| may return before a broadcast is received (eg. to return to an event | ||
| loop to await a message) and no code should follow the call to this | ||
| function. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's true AFAICT that no test currently in this suite defines a receiveBroadcast callback that uses its second argument, but that argument is provided. Here are some example runtime implementations:
And per the top of this file, relaxing the requirement to provide that second argument would need to come with a package.json version bump.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was rather watching this from the point of view of how the .broadcast function is called: it's only called within safeBroadcast and safeBroadcastAsync and never with a second parameter. So either the second parameter doesn't exist, or it has a third "None" value.
I wrote my implementation of the function based on these descriptions and assumed that this must be talking about passing a TypedArray to the receiver (as it has originally been AFAICT), and made the function check that the argument really is a (shared) TypedArray: whoops nope, an error is thrown. Okay, so it expects a SharedArrayBuffer and a second argument which I guess is a Number? Nope, still an error. What am I actually getting as a parameter? Oh, undefined every time.
Maybe this documentation isn't out of date per se, but it definitely is wrong. There is no second parameter currently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was rather watching this from the point of view of how the
.broadcastfunction is called: it's only called withinsafeBroadcastandsafeBroadcastAsyncand never with a second parameter. So either the second parameter doesn't exist, or it has a third "None" value.
Let's be clear on vocabulary—"parameter" is part of the signature of a function, while "argument" is part of an invocation. As mentioned above, no test in this repository currently specifies a receiveBroadcast function that uses a second argument (or indeed defines a second parameter at all), and no invocation of broadcast currently supplies a second argument—which, per normal ECMAScript semantics about absent arguments having value undefined, therefore means that all invocations of receiveBroadcast that are in conformance with INTERPRETING.md must receive either a single argument or a pair of arguments in which the second is undefined.
I wrote my implementation of the function based on these descriptions and assumed that this must be talking about passing a TypedArray to the receiver (as it has originally been AFAICT), and made the function check that the argument really is a (shared) TypedArray: whoops nope, an error is thrown. Okay, so it expects a SharedArrayBuffer and a second argument which I guess is a Number? Nope, still an error. What am I actually getting as a parameter? Oh,
undefinedevery time.
I agree that the text could be more clear. What I'm inferring based upon both it and actual implementations is that the second parameter is optional—broadcast is a function that takes a SharedArrayBuffer and an optional Int32 or BigInt and broadcasts the two values (the SharedArrayBuffer and a number or bigint or undefined) to all concurrent agents.
Maybe this documentation isn't out of date per se, but it definitely is wrong. There is no second parameter currently.
There most certainly is a second parameter, and any implementation that does not pass along a bigint or int32-representing number provided in an invocation of broadcast to the corresponding invocation(s) of receiveBroadcast would be out of conformance with the documented requirements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's be clear on vocabulary—"parameter" is part of the signature of a function, while "argument" is part of an invocation.
Ah, I apologise most dreadfully. This is unacceptable and truly heinous of me.
I agree that the text could be more clear. What I'm inferring based upon both it and actual implementations is that the second parameter is optional—
broadcastis a function that takes a SharedArrayBuffer and an optional Int32 or BigInt and broadcasts the two values (the SharedArrayBuffer and a number or bigint orundefined) to all concurrent agents.There most certainly is a second parameter, and any implementation that does not pass along a bigint or int32-representing number provided in an invocation of
broadcastto the corresponding invocation(s) ofreceiveBroadcastwould be out of conformance with the documented requirements.
I actually only only now realised to me that the second parameter is even supposed to be an integer. I had assumed the phrasing "BigInt or Int32" was listing two enumeration variants, instead of listing out two acceptable types for the second argument.
So; should I close the PR? Should I bump the package.json version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should repurpose this PR into a clarification of the existing requirements that does not change them.
| - **`broadcast`** - a function that takes a SharedArrayBuffer and | ||
| broadcasts it to all concurrent agents. The function blocks until all | ||
| agents have retrieved the message. Note, this assumes that all agents | ||
| that were started are still running. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise here, dropping the broadcast second parameter would need a version bump.
gibson042
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestions for repurposing this PR into a clarification of the existing requirements without changing them. Thanks for identifying the issues with current text!
| - **`receiveBroadcast`** - a function that takes a function and | ||
| calls the function when it has received a broadcast from the parent, | ||
| passing it the broadcast as two arguments, a SharedArrayBuffer and | ||
| an Int32 or BigInt. This function may return before a broadcast is received | ||
| (eg to return to an event loop to await a message) and no code should | ||
| follow the call to this function. | ||
| - **`receiveBroadcast`** - a function that takes a callback and | ||
| calls the callback when it has received a broadcast from the parent, | ||
| passing it the broadcast as a single SharedArrayBuffer. This function | ||
| may return before a broadcast is received (eg. to return to an event | ||
| loop to await a message) and no code should follow the call to this | ||
| function. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should repurpose this PR into a clarification of the existing requirements that does not change them.
| - **`broadcast`** - a function that takes a SharedArrayBuffer and | ||
| broadcasts it to all concurrent agents. The function blocks until all | ||
| agents have retrieved the message. Note, this assumes that all agents | ||
| that were started are still running. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - **`broadcast`** - a function that takes a SharedArrayBuffer and | |
| broadcasts it to all concurrent agents. The function blocks until all | |
| agents have retrieved the message. Note, this assumes that all agents | |
| that were started are still running. | |
| - **`broadcast`** - a function that takes a SharedArrayBuffer and optional | |
| int32-compatible number or arbitrary bigint, and broadcasts those two | |
| values to all concurrent agents (when the second argument is absent, the | |
| broadcast may either preserve its absence or replace it with `undefined`). | |
| The function blocks until all agents have retrieved the message. Note, | |
| this assumes that all agents that were started are still running. |
| - **`getReport`** - a function that reads an incoming string from any | ||
| agent, and returns it if it exists, or returns `null` otherwise. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - **`getReport`** - a function that reads an incoming string from any | |
| agent, and returns it if it exists, or returns `null` otherwise. | |
| - **`getReport`** - a function that reads an incoming string from any | |
| agent, and returns it if it exists, or returns `null` otherwise. |
| - **`monotonicNow`** - a function that returns a value that conforms to | ||
| [`DOMHighResTimeStamp`] and is produced in such a way that its | ||
| semantics conform to **[Monotonic Clock]**. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - **`monotonicNow`** - a function that returns a value that conforms to | |
| [`DOMHighResTimeStamp`] and is produced in such a way that its | |
| semantics conform to **[Monotonic Clock]**. | |
| - **`monotonicNow`** - a function that returns a value that conforms to | |
| [`DOMHighResTimeStamp`] and is produced in such a way that its semantics | |
| conform to **[Monotonic Clock]**. |
| - **`receiveBroadcast`** - a function that takes a callback and | ||
| calls the callback when it has received a broadcast from the parent, | ||
| passing it the broadcast as a single SharedArrayBuffer. This function | ||
| may return before a broadcast is received (eg. to return to an event | ||
| loop to await a message) and no code should follow the call to this | ||
| function. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - **`receiveBroadcast`** - a function that takes a callback and | |
| calls the callback when it has received a broadcast from the parent, | |
| passing it the broadcast as a single SharedArrayBuffer. This function | |
| may return before a broadcast is received (eg. to return to an event | |
| loop to await a message) and no code should follow the call to this | |
| function. | |
| - **`receiveBroadcast`** - a function that takes a callback to be invoked | |
| with the SharedArrayBuffer and optional number or bigint from each | |
| invocation of `$262.agent.broadcast` in the parent when that broadcast | |
| is received. `receiveBroadcast` may return before a broadcast is | |
| received (e.g. to enter an event loop awaiting messages), and no code | |
| should follow its invocation. |
receiveBroadcastdoes not take or broadcast a second parameter telling the intended type of the shared TypedArray; this is internally encoded in each test in the code they pass toagent.start.While here I cleared up the language, removed errant characters, fixed up some line wraps here and there, and fixed the format of the markdown reference links used for timestamp and monotonic clock (they had a meaningless extra
[]tacked at the end).