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

add Receiver::obtain_sender() and Sender::obtain_receiver() methods #762

Closed
wants to merge 2 commits into from
Closed

add Receiver::obtain_sender() and Sender::obtain_receiver() methods #762

wants to merge 2 commits into from

Conversation

amab8901
Copy link
Contributor

@amab8901 amab8901 commented Dec 22, 2022

Solves the 2nd checkbox on #715

@kevinaboos
Copy link
Member

kevinaboos commented Dec 22, 2022

Hi @amab8901, thanks for this PR and thanks for continuing to work on this!

Apologies, but I think you may have also misunderstood the purpose or intent of this feature. Now that we properly handle disconnection, we also want to allow re-connection for the goal of fault recovery.

Here's an example illustrating what we need.

  1. The channel is operating normally.
  2. Some fault occurs in a sending task, causing the last remaining Sender to be dropped.
  • The channel is now disconnected.
  1. Another task comes along and wishes to connect to the channel to send some data through it (e.g., the failed task was respawned).
  2. Here: the new task needs obtain a reference to the channel in order to acquire a Sender object. But the only thing that still exists is one (or more) Receiver objects. Currently there is no way for that new task to re-obtain a new Sender endpoint when Receivers are the only thing that still exist.

Thus, what we need is a function like so (pseudocode):

impl Receiver ... {
    pub fn obtain_sender(&self) -> Sender ... {
        ....
    }

and the inverse for Sender::obtain_receiver(...) --> Receiver. Hint: you can re-use the code in the Clone implementation that you added in #707.

In contrast, your current changeset offers functions that return the set of tasks waiting to send or receive. There's no use case for this, and it's also a safety/correctness violation because we wouldn't want anyone else except the internal channel logic to modify that set of waiting senders/receivers.


It would also be good to concurrently add a test case for this to the test_channel app (or as a separate app) that demonstrates how this functionality could be used and tests it thoroughly to prove that such a fault can be recovered from. If you attempted to implement such a test, you'd quickly see what kind of functions are needed and why your current approach isn't quite in the right direction.

@kevinaboos
Copy link
Member

I just realized that some of these functions were added to the async_channel in #668, which was just merged in. However, it doesn't properly handle the connection status and count of senders/receivers, so you can still make the improvements here in this PR.

@amab8901
Copy link
Contributor Author

amab8901 commented Dec 23, 2022

How do I use the test_channel app, or any app in Theseus for that matter? I tried running cargo doc and cargo test inside the test_channel app but neither seems to work

@amab8901
Copy link
Contributor Author

I changed the obtain_sender() and obtain_receiver(). Hope it's good now

@kevinaboos
Copy link
Member

How do I use the test_channel app, or any app in Theseus for that matter? I tried running cargo doc and cargo test inside the test_channel app but neither seems to work

You can run them in the Theseus terminal. By default, tests are not included in the build, so you can either explicitly include it in the build or include everything in the build with make full.

This is a relatively recent change to the build system, so it hasn't yet made it into the book. But it is documented in the Makefile as well as in the theseus_features crate, which you can more about here: https://www.theseus-os.com/Theseus/doc/theseus_features/index.html#how-to-customize-what-is-included-in-a-theseus-build

@kevinaboos
Copy link
Member

I changed the obtain_sender() and obtain_receiver(). Hope it's good now

If you take a look at the Files changed tab on this PR, you'll see that there are still some merge conflict sections left in the code, which means you didn't resolve the conflicts. You can refer to your last PR where I described how to address that, or simply search the web for git tutorials on resolving merge conflicts.

@amab8901
Copy link
Contributor Author

amab8901 commented Dec 24, 2022

I encountered an interesting problem. Here's how it looks like:

$ git branch
  703-async-channel-disconnection
  710-port-TSC-to-time
  710-port-clocks-to-new-API
  doc-index-time
* theseus_main
$ git status
On branch theseus_main
Your branch is up to date with 'origin/theseus_main'.

nothing to commit, working tree clean
$ cargo doc
error: failed to load source for dependency `backtrace`

Caused by:
  Unable to update /home/amab/src/Theseus/ports/backtrace

Caused by:
  failed to read `/home/amab/src/Theseus/ports/backtrace/Cargo.toml`

Caused by:
  No such file or directory (os error 2)

I've been using cargo doc thus far as a substitute for cargo test to check if there are any errors in the code. But now it doesn't seem to work anymore.

I tried re-cloning Theseus from scratch into a temporary directory for comparison. It works fine there.

I think it's caused by me playing around with git reset --hard and git revert

What would be the best approach to solving this? Do I need to delete my current project locally and restart from scratch, or is there a better approach?

@amab8901 amab8901 closed this by deleting the head repository Dec 27, 2022
@kevinaboos
Copy link
Member

Well you didn't need to delete your repo, but that's fine, we'll continue on your new PR.

@amab8901
Copy link
Contributor Author

Well you didn't need to delete your repo, but that's fine, we'll continue on your new PR.

I was just having some issues starting the cargo doc tests (now resolved) and wanted to try if starting the repo from scratch might help

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants