Skip to content
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

TrapCaps for async host, synchronous guest relationships #3171

Merged
merged 31 commits into from
Jul 19, 2021

Conversation

michaelfig
Copy link
Member

@michaelfig michaelfig commented May 25, 2021

Fixes #2846 while we're in the neighbourhood.

As discussed with @erights and @kriskowal, this implements the possibility for a Host captp endpoint to serve synchronous Trap requests made by a trusting Guest captp endpoint. See test-trap.js for how it works.

Essentially, Alice's can set opts.giveTrapReply and serve a makeTrapHandler(name, object), which when given the right opts.takeTrapReply on Bob's side, permits Bob to call Trap(remotable)[prop](...args) (or Trap.get(remotable)[prop] or Trap(remotable)(...args)), blocking and waiting for Alice to asynchronously resolve the answer normally, but Bob sees the result as if it were synchronous.

This must only be used when Bob can and must depend on Alice's correct functioning, such as when Alice is a device-driver-like service. Using it in other ways leads to madness.

@michaelfig michaelfig added the captp package: captp label May 25, 2021
@michaelfig michaelfig requested review from kriskowal and erights May 25, 2021 04:36
@michaelfig michaelfig self-assigned this May 25, 2021
@michaelfig michaelfig force-pushed the mfig-syncable-captp branch from b596232 to 61f6a30 Compare May 25, 2021 04:44
@michaelfig michaelfig force-pushed the mfig-syncable-captp branch from 61f6a30 to 3989998 Compare May 25, 2021 04:56
Copy link
Member

@kriskowal kriskowal left a comment

Choose a reason for hiding this comment

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

I’ve left some comments. I definitely need to read what’s already in CapTP top-to-bottom to have the knowledge necessary to give this a hearty stamp. What I see looks fine, except a strong feeling handler behaviors shouldn’t be conflated into a single utility function.

I would like to see a code sample of how this would interact with SharedBuffer and Atomics, if not a working example.

packages/captp/lib/sync.js Outdated Show resolved Hide resolved
packages/captp/lib/sync.js Outdated Show resolved Hide resolved
packages/captp/test/test-sync.js Outdated Show resolved Hide resolved
packages/captp/lib/ts-types.d.ts Outdated Show resolved Hide resolved
@michaelfig
Copy link
Member Author

I would like to see a code sample of how this would interact with SharedBuffer and Atomics, if not a working example.

I may supply such an example, but it will be only a part of a PR stacked on top of this PR. The simplest correct implementation of Worker/postMessage/SharedArrayBuffer/Atomics is at least 100 lines of code, even when given the foundation in this PR.

@michaelfig michaelfig force-pushed the mfig-syncable-captp branch 4 times, most recently from 41a8128 to 6a340c9 Compare May 31, 2021 02:01
@michaelfig michaelfig marked this pull request as draft June 2, 2021 16:02
@michaelfig
Copy link
Member Author

I would like to see a code sample of how this would interact with SharedBuffer and Atomics, if not a working example.

@erights please hold off on this review until I provide an example as requested by @kriskowal. The factoring of such an example will inform what API captp should provide.

@michaelfig michaelfig force-pushed the mfig-syncable-captp branch from edea844 to 4f60874 Compare June 3, 2021 00:15
@michaelfig
Copy link
Member Author

@erights and @kriskowal, this is now fully-baked and ready for a review. Notably, the test-sync.js tests demonstrate the use of Workers communicating via SharedArrayBuffer with Atomics.

@michaelfig michaelfig marked this pull request as ready for review June 3, 2021 00:17
@michaelfig michaelfig force-pushed the mfig-syncable-captp branch from 4f60874 to b1816d1 Compare June 3, 2021 00:24
@michaelfig michaelfig changed the title Syncable captp CapTraps Jun 9, 2021
@michaelfig michaelfig changed the title CapTraps TrapCaps for async host, synchronous guest relationships Jun 9, 2021
@michaelfig michaelfig force-pushed the mfig-syncable-captp branch from da49aef to 1fdb173 Compare June 9, 2021 03:12
@michaelfig michaelfig force-pushed the mfig-syncable-captp branch 3 times, most recently from 525fef3 to 0f5a1e6 Compare June 17, 2021 01:08
Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

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

Just some review comments so far.

packages/captp/lib/types.js Outdated Show resolved Hide resolved
packages/captp/lib/types.js Outdated Show resolved Hide resolved
packages/captp/lib/types.js Outdated Show resolved Hide resolved
packages/captp/lib/ts-types.d.ts Outdated Show resolved Hide resolved
packages/captp/lib/ts-types.d.ts Outdated Show resolved Hide resolved
packages/captp/lib/trap-driver.js Outdated Show resolved Hide resolved
packages/captp/src/trap.js Show resolved Hide resolved
packages/captp/src/trap-driver.js Outdated Show resolved Hide resolved
packages/captp/src/captp.js Outdated Show resolved Hide resolved
packages/captp/src/index.js Show resolved Hide resolved
Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

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

If I remain tardy in reviewing this PR (sorry), would it be useful to extract the #2846 fix as a distinct PR so it can go in asap?

packages/captp/src/captp.js Outdated Show resolved Hide resolved
packages/captp/src/captp.js Show resolved Hide resolved
@michaelfig michaelfig force-pushed the mfig-syncable-captp branch from 44ba004 to a350b9d Compare July 16, 2021 03:07
Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

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

Comments so far.

packages/captp/src/types.js Outdated Show resolved Hide resolved
packages/captp/src/types.js Outdated Show resolved Hide resolved
packages/captp/src/types.js Outdated Show resolved Hide resolved
packages/captp/src/ts-types.d.ts Outdated Show resolved Hide resolved
packages/captp/src/ts-types.d.ts Outdated Show resolved Hide resolved
packages/captp/src/trap.js Show resolved Hide resolved
packages/captp/src/trap.js Show resolved Hide resolved
packages/captp/test/worker.cjs Show resolved Hide resolved
packages/captp/test/worker.js Outdated Show resolved Hide resolved
packages/captp/test/worker.js Show resolved Hide resolved
Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

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

The loopback case doesn't make any use of iteration, because of course not. This has me wondering whether the generic types should hide rather that expose the iterators on both side, so that it is only the business of an individual makeGuest/makeHost pair (like atomics) whether it needs iterators or not.

packages/captp/src/atomics.js Outdated Show resolved Hide resolved
packages/captp/src/atomics.js Outdated Show resolved Hide resolved
packages/captp/src/types.js Outdated Show resolved Hide resolved
packages/captp/src/types.js Outdated Show resolved Hide resolved
packages/captp/src/types.js Outdated Show resolved Hide resolved
packages/captp/src/atomics.js Outdated Show resolved Hide resolved
packages/captp/src/atomics.js Outdated Show resolved Hide resolved
packages/captp/src/trap.js Show resolved Hide resolved
packages/captp/src/trap.js Show resolved Hide resolved
packages/captp/src/captp.js Show resolved Hide resolved
@erights
Copy link
Member

erights commented Jul 19, 2021

#3171 (comment) is my only comment that is likely a bug. The others are questions, suggestions, or things to discuss.

Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

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

LGTM!

But please don't merge until we talk

packages/captp/src/captp.js Outdated Show resolved Hide resolved
packages/captp/src/captp.js Outdated Show resolved Hide resolved
packages/captp/src/captp.js Show resolved Hide resolved
@michaelfig michaelfig enabled auto-merge July 19, 2021 23:25
@michaelfig michaelfig merged commit 006d030 into master Jul 19, 2021
@michaelfig michaelfig deleted the mfig-syncable-captp branch July 19, 2021 23:52
@dckc dckc mentioned this pull request Sep 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
captp package: captp
Projects
None yet
Development

Successfully merging this pull request may close these issues.

is captp pipelining result promises correctly?
3 participants