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

Connection and multicast semantics for command device #748

Closed
michaelfig opened this issue Mar 20, 2020 · 4 comments
Closed

Connection and multicast semantics for command device #748

michaelfig opened this issue Mar 20, 2020 · 4 comments
Assignees
Labels
cosmic-swingset package: cosmic-swingset SwingSet package: SwingSet

Comments

@michaelfig
Copy link
Member

The current SwingSet command device provides a simple request/response(or rejection) implementation and a separate sendBroadcast implementation. This is not very flexible for splitting up the HTTP/WebSocket handlers into separate objects, each with their own set of peers that are subscribing to them.

It would be useful to add the notion of a "connection" to the command device. Essentially, the SwingSet host could call command.setConnection(connectionID, sendFn = undefined) which would associate or remove a sendFn with a connectionID (a Nat).

inboundCommand would then take an optional second argument, the connectionID. Then, on the kernel side of the device, these functions would result in callbacks of:

// new connection lifecycle callbacks
function connectCallback(connectionID) {}
function disconnectCallback(connectionID) {}
 // connectionID is optional, and its absence means this is a one-shot calling context
function inboundCallback(count, bodyString, connectionID = undefined) {}

The command device outer half would translate these calls into one of the following:

SO(inboundHandler).connect(connectionHandle) // or
SO(inboundHandler).disconnect(connectionHandle) // or
SO(inboundHandler).inbound(Nat(count), body, connectionHandle)

Each connectionHandle would be an opaque object that the outer half of the command device would associate with internal connectionIDs, for ocap hygiene (so that the client code doesn't see the IDs). Then, the outer half of the command device would provide:

sendMulticast(obj, connectionHandles) {
  // Moral equivalent of.
  connectionHandles.forEach(connectionHandle => {
    const connectionID = connections.get(connectionHandle);
    const sendFn = sendFns.get(connectionID);
    try {
      sendFn(obj);
    } catch (e) {
      // The connection is either gone (lost after restart), or the sendFn failed, so disconnect.
      connections.delete(connectionHandle);
      sendFns.delete(connectionID);
      SO(inboundHandler).disconnect(connectionHandle);
    }
  });
}

When the host starts up, it could get a list of active (i.e. saved in the device state) connectionIDs, and explicitly close them (or for resumable connections, associate a new sendFn with them).

We may want to remove the old sendBroadcast as it assumes all connections are equal and is basically an ambient authority.

@michaelfig
Copy link
Member Author

@warner, @FUDCo can you please review this design? I'd like to clean up the HTTP/WebSocket handling in ag-solo, which is currently unclear.

@michaelfig michaelfig added cosmic-swingset package: cosmic-swingset SwingSet package: SwingSet labels Mar 20, 2020
@michaelfig michaelfig self-assigned this Mar 20, 2020
michaelfig added a commit that referenced this issue Mar 20, 2020
This anticipates support for #748
in the command device.
@warner
Copy link
Member

warner commented Mar 22, 2020

Hm. My instinct is to be anxious about extending the communication pathways with non-CapTP-ish channels, because of how the other side is not obligated to be as deterministic and "waterken like" as real Vats are. But we've discussed that before and I think I came away thinking it might be possible to pull off.

We definitely need to pay close attention to the impedance-mismatch between the external notion of "connection" (TCP or WebSocket connections, which can appear or drop at any time), and the vat's notions (where vats only hear about messages from devices). In particular the way we manage state: the devices will keep some state on the heap (including references to HTTP channel objects, which obviously can't go anywhere else), and some state in the kernel. They must be prepared to wake up after a restart and deal with the difference. It's like someone holding a phone in their hand and then they blink and the phone is gone, but they still remember things about the conversation they were having a moment ago.

Removing sendBroadcast sounds fine to me. I'm not sure we need a special sendMulticast (rather then just calling send-unicast multiple times), but I do believe we made a distinction between sending a message in response to some command, and sending a message as an unprompted update. If that's the distinction we really care about, maybe we should have methods named send and respond or something.

@michaelfig
Copy link
Member Author

@warner, thanks for your review. I'd suggest renaming what I've called "connection" to be just "channel" (to reflect the difference between external notions and the command device's notion) and changing sendMulticast to just send. I'd prefer to keep the send channelHandles as an array, to reduce the number of device calls needed to multicast to many endpoints.

I like the distinction we already have between the new send and deliverResponse (which we could just rename to be respond as you have suggested). You can't send to commands that come in without a channelHandle, you can only respond to them (which might not even be supported on certain kinds of commands).

michaelfig added a commit that referenced this issue Mar 22, 2020
This anticipates support for #748
in the command device.
@michaelfig michaelfig mentioned this issue Apr 5, 2020
@michaelfig
Copy link
Member Author

This was an interesting discussion, but in order to make progress on this we're going to have to come up with a better device/design.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cosmic-swingset package: cosmic-swingset SwingSet package: SwingSet
Projects
None yet
Development

No branches or pull requests

2 participants