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

Linear handles tracking issue #1854

Closed
tiziano88 opened this issue Feb 1, 2021 · 8 comments
Closed

Linear handles tracking issue #1854

tiziano88 opened this issue Feb 1, 2021 · 8 comments
Labels
enhancement New feature or request lang/Rust

Comments

@tiziano88
Copy link
Collaborator

tiziano88 commented Feb 1, 2021

Here is my game plan for making handles linear types:

  1. Add handle_clone syscall (Add handle_clone to ABI and runtime. #1782)
  2. Allow SDK functions (channel_read, channel_write) to use borrowed handles instead of owned ones (Allow passing borrowed handles to SDK functions. #1843)
  3. Add linear-handles feature to oak_io that optionally makes handle types defined there linear (Add linear-handles feature to oak_io #1850)
  4. Add linear-handles feature to SDK for compatibility with the oak_io feature (Add linear-handles feature to SDK. #1884)
  5. Migrate individual example modules to use the new feature
  6. Migrate runtime to use linear handles
  7. Remove the features and make their behaviour the default

Of course we could also do all of this at once (e.g. https://github.com/wildarch/oak/tree/handle_clone_drop_impls), but I suspect that would be a very tough PR to land, because it touches so much of the codebase.

Originally posted by @wildarch in #1686 (comment)

@tiziano88 tiziano88 added enhancement New feature or request lang/Rust labels Feb 1, 2021
@tiziano88
Copy link
Collaborator Author

Pulling this into its own issue as there are quite a few moving parts already.

I wonder if we should have a more detailed RFC, discussing pros / cons, alternatives considered, and what effect it has on the current codebase.

It seems to me that #1850 is a good proof of concept already that shows that it is feasible and quite elegant.

cc @daviddrysdale

@wildarch
Copy link
Contributor

wildarch commented Feb 1, 2021

Thanks for this, I agree it makes sense to spin this off into a separate issue 😄 .

I can write up an RFC for this if that is appreciated, but to me it seems quite clear-cut that linear handles are an improvement over the current approach: they correspond to Rust's ownership model and prevent leaks. Are there many alternative ideas to consider here? Keeping the current system would be one, I'm not sure if there are others.

It would have quite an impact on the codebase though, maybe that in itself would be a good reason to write up an RFC? I'm not sure.

@tiziano88
Copy link
Collaborator Author

Perhaps no need for an actual RFC, but could you describe roughly what the behaviour would be for read and write handles, for instance? e.g. I would expect that only write handles can be cloned, is that what you are thinking too already? Also what happens when a message with handles is cloned? I guess all the handles inside it are cloned too, is that something that will come naturally, or will it require additional logic in the code generation or elsewhere?

@wildarch
Copy link
Contributor

wildarch commented Feb 5, 2021

The major changes for read and write handles would be:

  • No more Copy impl. SDK functions such as channel_read should take a &ReadHandle instead of an owned ReadHandle, to avoid unnecessary cloning.
  • Clone is implemented through handle_clone ABI call. To make .clone() safe to call, creating a ReadHandle or WriteHandle from an arbitrary raw handle will become an unsafe operation. Same goes for mutating the inner handle value, but we can leave a raw_handle() getter for people that need it.
  • Drop is implemented using channel_close. Any errors return here are dropped, since Drop is only to prevent leaks, and most error cases here are benign.

I was thinking read handles would also be clone-able, but I'm curious to know why they should not be? 😄

Cloning a message with handles would work fine, as it will call the regular Clone impl on the handles. We will need to be a bit careful when actually sending a message with handles over a channel. Encoding them into an oak message clones all the handles, so we must be careful to also explicitly drop the raw handles sent to the runtime.
The above would probably be much more difficult if read handles are not clone-able, so we'll need to rethink that if we choose to go that way.

@tiziano88
Copy link
Collaborator Author

I was thinking read handles would also be clone-able, but I'm curious to know why they should not be?

See #1197; basically, if two nodes end up with read handles to the same underlying channel, they can communicating with each other bypassing IFC simply by reading or not reading from the underlying channel, since reading is a destructive operation (e.g. if there is exactly one message in a channel, the first one who reads from it will remove it from the channel, and the next node will see it as empty).

Note that there is also a proposal around to use handles as "references" to immutable pieces of data (i.e. data may be "packed" into an immutable singleton channel, and nodes may dereference it by "unpacking" it, as long as their label allows it). In this case it would be fine to clone read handles to those sort of reference-like channels. @rbehjati is thinking about those aspects so she may have more details.

@rbehjati
Copy link
Contributor

After a discussion, @wildarch and I agree that the reference-like channels for field-level labels can be left out for now.

So, to address the issue discussed in #1197, read handles will be made linear: they won't be cloneable, and when cloning a message with handles, the cloned message gets the ownership of the read handles (IIUC). However, after looking at #1197 again, it seems that the conclusion is that read handles do not need to be linear, instead the label check should be tweaked: #1197 (comment)
Is it still required to make read handles linear? My conclusion is that they could be cloneable just like write handles.

@tiziano88
Copy link
Collaborator Author

Thanks for looking into this @rbehjati and @wildarch . The suggestion of checking labels in both directions when reading from a channel (in addition to when writing to it, of course) makes sense, so if all the examples still work under that assumption, we can go ahead with that!

@wildarch
Copy link
Contributor

That's great, thanks for revisiting #1197 @rbehjati! The linear-handles feature in oak_io currently makes ReadHandle types Cloneable, so I believe that can go ahead as originally planned now 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request lang/Rust
Projects
None yet
Development

No branches or pull requests

3 participants