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.

Sign up for GitHub

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

Jump to bottom

Make pseudo-Nodes use Handles not raw channel references #767

Merged
merged 15 commits into from
Mar 26, 2020
Merged

Make pseudo-Nodes use Handles not raw channel references #767

merged 15 commits into from
Mar 26, 2020

Conversation

daviddrysdale
Copy link
Contributor

@daviddrysdale daviddrysdale commented Mar 25, 2020 •
edited
Loading

Checklist

  • Pull request affects core Oak functionality (e.g. runtime, SDK, ABI)
    • I have written tests that cover the code changes.
    • I have checked that these tests are run by Cloudbuild
    • I have updated documentation accordingly.
    • I have raised an issue to
      cover any TODOs and/or unfinished work.
  • Pull request includes prototype/experimental work that is under
    construction.

@daviddrysdale
runtime: put OakStatus into ReadResult
9d66669
@daviddrysdale
runtime: adjust name and return for ChannelClose
bec5e3a
@daviddrysdale
runtime: add and use OakNode::ChannelRead
5211445
@daviddrysdale
runtime: add and use OakNode::ChannelWrite
2c22484
@daviddrysdale
runtime: add and use HandleCloser RAII class
845afe6
@daviddrysdale
runtime: use handles not refs on channel read
fd35557
@daviddrysdale
runtime: move Invocation and users to handles
bbe2745 
Move the Invocation object to hold auto-closing channel handles
rather than raw channel references, and adapt the two pseudo-Nodes
that use Invocations (gRPC client and storage) appropriately.
@daviddrysdale
runtime: use handles not refs on channel write
180ce68
@daviddrysdale
runtime: add and use OakNode::NodeCreate
734a78a
@daviddrysdale
runtime: add and use OakNode::ChannelCreate
66e93e4
@daviddrysdale
runtime: hide OakNode's reference to runtime
1d829d1
@daviddrysdale
runtime: make AddChannel private to runtime
10d2bfe
@daviddrysdale
runtime: pass initial handles around
6414cc2 
At start of day there are exactly two Nodes that need to be started,
the gRPC server pseudo-Node and the initial Application Wasm Node.
Hang on the OakNode reference and initial Handle for both of these,
and move OakNode::Start() to always take a Handle.  This means that
OakNode::SingleHandle() can be removed.
@daviddrysdale
runtime: move gRPC server Node to use handles
a03c011
@daviddrysdale
runtime: make raw channel functions private
43d86bd 
Derived classes should now always use Handles to refer to channels,
not raw channel references.
@daviddrysdale daviddrysdale marked this pull request as ready for review March 25, 2020 15:40
@daviddrysdale
Copy link
Contributor Author

Probably easiest to review commit-by-commit.

@tiziano88
Copy link
Collaborator

I just note that this seems quite a large change to write (well, that's already done now) and review (still ongoing), for code that will (hopefully) not be around for that long! Or do you expect this code to continue existing as C++ linked to the Rust runtime in the long term @daviddrysdale ?

@daviddrysdale
Copy link
Contributor Author

This is definitely needed for #729, and is very likely needed for #724 (unless we're expecting to get three types of pseudo-Node functionality re-implemented in Rust in the very near future).

Taken commit-by-commit, it's probably not quite as big a change as it first appears (and it wasn't that big to write – under a day). The core of it is teasing apart and moving code from wasm_node.cc to oak_node.cc, so the former is left with Wasm-specific functionality and the latter is the only place that deals with raw channel references.

conradgrobler
conradgrobler approved these changes Mar 26, 2020
View reviewed changes
Copy link
Collaborator

@conradgrobler conradgrobler left a comment

Choose a reason for hiding this comment

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

LGTM

tiziano88
tiziano88 approved these changes Mar 26, 2020
View reviewed changes
Copy link
Collaborator

@tiziano88 tiziano88 left a comment

Choose a reason for hiding this comment

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

LGTM, though I am confused by the difference between Message and NodeMessage and the need to have both around. I will try to understand the code better, but I don't want that to hold this PR. If you have a high-level summary @daviddrysdale (in addition to what's already in the PR) that would be useful!

@daviddrysdale
Copy link
Contributor Author

The existing Message class holds a vector of ChannelHalf objects, which hold explicit memory references to channel objects (shared_ptr<MessageChannel>, inside some wrappers) – and these are very runtime-internal pointers

The NodeMessage class is supposed to be equivalent, but with a Node-relative Handle (uint64_t) in place of a pointer – it's a Message as seen by a Node that does not have access to the internals of the runtime. (Each such object is also specific to a particular Node, because the Handle numbering space is Node-relative.)

So by moving all of the pseudo-Node code to use NodeMessage rather than Message, it should make it easier to map to an FFI boundary that uses Handles for channel identification, analogously to the ABI.

@daviddrysdale daviddrysdale merged commit 8069863 into project-oak:master Mar 26, 2020
@daviddrysdale daviddrysdale deleted the pseudo branch March 26, 2020 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@conradgrobler conradgrobler conradgrobler approved these changes

@tiziano88 tiziano88 tiziano88 approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@daviddrysdale @tiziano88 @conradgrobler @googlebot