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

Enable tokio io driver #171

Merged
merged 14 commits into from
Apr 3, 2024
Merged

Conversation

HolyShitMan
Copy link
Contributor

Pull request includes:

  • activation of tokio io driver to enable net, process & signals within runtime (for our use-case we need net)
  • some additional formating stuff

@mcches
Copy link
Contributor

mcches commented Mar 26, 2024

This has come up a couple of times in the past. We intentionally don't enable io to avoid confusion between tokio::net and turmoil::net types. Can I ask why you aren't using turmoil::net types in your tests and for a little more information about your use case?

@HolyShitMan
Copy link
Contributor Author

We certainly understand this intention. We have currently two use-cases, that use the tokio io types:

  1. Manual GUI testing framework that uses turmoil internally and exposes one socket externally to attach GUI.
  2. Tests that write files onto the HDD (missed this in my initial comment onto the PR)

Maybe a compromise could be to hide the IO driver activation behind a feature flag?!

@mcches
Copy link
Contributor

mcches commented Mar 26, 2024

  1. Manual GUI testing framework that uses turmoil internally and exposes one socket externally to attach GUI.

Interesting! I'd love to hear more about this or see a demo.

  1. Tests that write files onto the HDD (missed this in my initial comment onto the PR)

This should be fine if you use tokio fs as that doesn't use the io reactor.

Maybe a compromise could be to hide the IO driver activation behind a feature flag?!

I'd be more comfortable doing it like this.

@mcches
Copy link
Contributor

mcches commented Mar 26, 2024

You could also smuggle messages out via a channel and then run your socket independently of turmoil.

@HolyShitMan
Copy link
Contributor Author

You could also smuggle messages out via a channel and then run your socket independently of turmoil.

This of course could also be possible.
Any strong opinion from your side, whether to include this PR?

@HolyShitMan
Copy link
Contributor Author

We have currently two use-cases, that use the tokio io types:

I ask the college again on his problems and I misunderstood him. The problem weren't the files, but he ran a command which uses pipes internally. And this led to a tokio crash without IO feature activated.

@mcches
Copy link
Contributor

mcches commented Mar 26, 2024

You could also smuggle messages out via a channel and then run your socket independently of turmoil.

This of course could also be possible. Any strong opinion from your side, whether to include this PR?

If it unblocks you and it isn't too much of a hassle I'd prefer that approach. One reason for this is that at some point down the line we'd like to incorporate this into tokio itself, at which point you'd set a flag on the runtime and you get one or the other. I'm not sure when that will be, but if we can avoid it now that would save you some upgrade headache later.

@HolyShitMan
Copy link
Contributor Author

I talked to all my colleges, that are involved in this issue of needing IO within unit tests, and the consensus was, that by not using the tokio IO driver the short and medium term effort is to high and our code base readability decreases massively.
Still, we fully understand your point, and if you're not inclined to merge this PR, we propose two alternatives:

  1. I withdraw this PR and create a new one, introducing an additional constructor that accepts a user-created tokio runtime as a parameter. Maybe this aligns better with your long-term goals.
  2. If this isn't feasible, we'll continue using our local turmoil fork

@mcches
Copy link
Contributor

mcches commented Mar 27, 2024

Let's do a Builder option and proceed with enabling it. This has the advantage of being very explicit and allows for tests to have it on and off in the same crate.

@HolyShitMan
Copy link
Contributor Author

Not completely sure about everything, e.g., building no_software runtime with Config::default();, yet count this as a first version to discuss about.

Copy link
Contributor

@mcches mcches left a comment

Choose a reason for hiding this comment

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

Looks pretty good! A couple minor comments.

src/builder.rs Outdated Show resolved Hide resolved
src/rt.rs Outdated
@@ -48,14 +51,17 @@ pub(crate) struct Rt<'a> {
/// Optional handle to a host's software. When software finishes, the handle is
/// consumed to check for error, which is propagated up to fail the simulation.
handle: Option<JoinHandle<Result>>,

/// Configuration of simulation
sim_cfg: Config,
Copy link
Contributor

Choose a reason for hiding this comment

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

If you switch to just the enable_io boolean here you don't need to worry about a default cfg for the no software case.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see this in the recent changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh sorry. I completely misunderstood your comment 🤦
But now, after reading it again of course I will change this appropriately.
Sorry again.

Copy link
Contributor

@mcches mcches left a comment

Choose a reason for hiding this comment

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

Lgtm!

src/rt.rs Outdated Show resolved Hide resolved
src/builder.rs Show resolved Hide resolved
@mcches mcches merged commit c5b595d into tokio-rs:main Apr 3, 2024
3 checks passed
@mcches
Copy link
Contributor

mcches commented Apr 3, 2024

Merged. I will release this by eow.

@HolyShitMan
Copy link
Contributor Author

thx for your work

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