Skip to content

Commit

Permalink
Writing "Prevent cycles" section.
Browse files Browse the repository at this point in the history
  • Loading branch information
Gohla committed Oct 18, 2023
1 parent e38e225 commit 0e3a302
Show file tree
Hide file tree
Showing 6 changed files with 249 additions and 2 deletions.
2 changes: 1 addition & 1 deletion tutorial/src/3_min_sound/6_hidden_dep/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ In this section, we will:

1) Create tests to showcase the hidden dependency problem.
2) Prevent hidden dependencies by checking for them at runtime, fixing the issue.
3) Improve and add additional tests
3) Improve and add additional tests.

## Test to showcase the issue

Expand Down
142 changes: 142 additions & 0 deletions tutorial/src/3_min_sound/7_cycle/a_task.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,142 @@
use std::io::{BufWriter, ErrorKind, Read, Stdout};
use std::path::PathBuf;

use dev_shared::write_until_modified;
use pie::{Context, Pie, Task};
use pie::stamp::FileStamper;
use pie::tracker::CompositeTracker;
use pie::tracker::event::EventTracker;
use pie::tracker::writing::WritingTracker;

/// Testing tracker composed of an [`EventTracker`] for testing and stdout [`WritingTracker`] for debugging.
pub type TestTracker<T> = CompositeTracker<EventTracker<T, <T as Task>::Output>, WritingTracker<BufWriter<Stdout>>>;
pub fn test_tracker<T: Task>() -> TestTracker<T> {
CompositeTracker(EventTracker::default(), WritingTracker::with_stdout())
}

/// Testing [`Pie`] using [`TestTracker`].
pub type TestPie<T> = Pie<T, <T as Task>::Output, TestTracker<T>>;
pub fn test_pie<T: Task>() -> TestPie<T> {
TestPie::with_tracker(test_tracker())
}

/// Testing extensions for [`TestPie`].
pub trait TestPieExt<T: Task> {
/// Require `task` in a new session, assert that there are no dependency check errors, then runs `test_assert_func`
/// on the event tracker for test assertion purposes.
fn require_then_assert(
&mut self,
task: &T,
test_assert_func: impl FnOnce(&EventTracker<T, T::Output>),
) -> T::Output;

/// Require `task` in a new session, asserts that there are no dependency check errors.
fn require(&mut self, task: &T) -> T::Output {
self.require_then_assert(task, |_| {})
}

/// Require `task` in a new session, then assert that it is not executed.
fn require_then_assert_no_execute(&mut self, task: &T) -> T::Output {
self.require_then_assert(task, |t|
assert!(!t.any_execute_of(task), "expected no execution of task {:?}, but it was executed", task),
)
}
/// Require `task` in a new session, then assert that it is executed exactly once.
fn require_then_assert_one_execute(&mut self, task: &T) -> T::Output {
self.require_then_assert(task, |t|
assert!(t.one_execute_of(task), "expected one execution of task {:?}, but it was not executed, or was executed more than once", task),
)
}
}
impl<T: Task> TestPieExt<T> for TestPie<T> {
fn require_then_assert(&mut self, task: &T, test_assert_func: impl FnOnce(&EventTracker<T, T::Output>)) -> T::Output {
let mut session = self.new_session();
let output = session.require(task);
assert!(session.dependency_check_errors().is_empty(), "expected no dependency checking errors, but there are \
dependency checking errors: {:?}", session.dependency_check_errors());
test_assert_func(&self.tracker().0);
output
}
}

/// Testing tasks enumeration.
#[derive(Clone, Eq, PartialEq, Hash, Debug)]
pub enum TestTask {
Return(&'static str),
ReadFile(PathBuf, FileStamper, Option<Box<TestTask>>),
WriteFile(Box<TestTask>, PathBuf, FileStamper),
ToLower(Box<TestTask>),
ToUpper(Box<TestTask>),
Sequence(Vec<TestTask>),
RequireSelf,
RequireA,
RequireB,
}
impl Task for TestTask {
type Output = Result<TestOutput, ErrorKind>;
fn execute<C: Context<Self>>(&self, context: &mut C) -> Self::Output {
match self {
TestTask::Return(string) => Ok(string.to_string().into()),
TestTask::ReadFile(path, stamper, origin) => {
if let Some(origin) = origin {
context.require_task(origin)?;
}
let mut string = String::new();
if let Some(mut file) = context.require_file_with_stamper(path, *stamper).map_err(|e| e.kind())? {
file.read_to_string(&mut string).map_err(|e| e.kind())?;
}
Ok(string.into())
}
TestTask::WriteFile(string_provider_task, path, stamper) => {
let string = context.require_task(string_provider_task.as_ref())?.into_string();
write_until_modified(path, string.as_bytes()).map_err(|e| e.kind())?;
context.provide_file_with_stamper(path, *stamper).map_err(|e| e.kind())?;
Ok(TestOutput::Unit)
}
TestTask::ToLower(string_provider_task) => {
let string = context.require_task(string_provider_task)?.into_string();
Ok(string.to_lowercase().into())
}
TestTask::ToUpper(string_provider_task) => {
let string = context.require_task(string_provider_task)?.into_string();
Ok(string.to_uppercase().into())
}
TestTask::Sequence(tasks) => {
for task in tasks {
context.require_task(task)?;
}
Ok(TestOutput::Unit)
}
TestTask::RequireSelf => context.require_task(&TestTask::RequireSelf),
TestTask::RequireA => context.require_task(&TestTask::RequireB),
TestTask::RequireB => context.require_task(&TestTask::RequireA),
}
}
}

/// [`TestTask`] output enumeration.
#[derive(Clone, Eq, PartialEq, Hash, Debug)]
pub enum TestOutput {
String(String),
Unit,
}
impl From<String> for TestOutput {
fn from(value: String) -> Self { Self::String(value) }
}
impl From<()> for TestOutput {
fn from(_: ()) -> Self { Self::Unit }
}
impl TestOutput {
pub fn as_str(&self) -> &str {
match self {
Self::String(s) => &s,
_ => panic!("{:?} does not contain a string", self),
}
}
pub fn into_string(self) -> String {
match self {
Self::String(s) => s,
_ => panic!("{:?} does not contain a string", self),
}
}
}
24 changes: 24 additions & 0 deletions tutorial/src/3_min_sound/7_cycle/b_test.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@


// Cycle tests

#[test]
#[should_panic(expected = "Cyclic task dependency")]
fn require_self_panics() {
let mut pie = test_pie();
pie.require(&RequireSelf).unwrap();
}

#[test]
#[should_panic(expected = "Cyclic task dependency")]
fn require_cycle_a_panics() {
let mut pie = test_pie();
pie.require(&RequireA).unwrap();
}

#[test]
#[should_panic(expected = "Cyclic task dependency")]
fn require_cycle_b_panics() {
let mut pie = test_pie();
pie.require(&RequireB).unwrap();
}
72 changes: 72 additions & 0 deletions tutorial/src/3_min_sound/7_cycle/index.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
# Prevent Cycles

```admonish warning title="Under construction"
This page is under construction.
```

In this section, we will fix the remaining correctness issue with cyclic tasks.

Didn't we already catch dependency graph cycles in the Incremental Top-Down Context section?
Yes, you remembered right!
However, there is a corner case that we didn't handle.
The issue is that we add a task dependency to the dependency graph only _after the task has finished executing_.
We do this because we need the output from executing the task to create the dependency.

But what would happen if we made a task that just requires itself?
Let's figure that out in this section, in which we will:

1) Add cyclic tasks to the testing tasks.
2) Create tests to showcase the cyclic task execution problem.
3) Prevent cycles by _reserving_ a task dependency before executing the task.
4) Improve and add additional tests.

## Add cyclic testing tasks

We don't have any testing tasks to easily construct different kinds of cycles yet, so we will add those first.

Modify `pie/tests/common/mod.rs`:

```diff2html linebyline
{{#include ../../gen/3_min_sound/7_cycle/a_task.rs.diff}}
```

We add the `RequireSelf` task which directly requires itself.
We also add the `RequireA` and `RequireB` tasks which require each other in a cycle.
We want to prevent both of these kinds of cycles.

## Add cycle tests

Now add tests that check whether requiring these tasks (correctly) panics due to cycles.

Modify `pie/tests/top_down.rs`:

```rust,
{{#include b_test.rs:3:}}
```

These test are simple: require the task and that's it.
Which of these tests will correctly result in a cyclic task dependency panic?

```admonish warning title="Infinite Recursion"
Running these tests will result in infinite recursion, but should quickly cause a stack overflow.
However, be sure to save everything in the event your computer becomes unresponsive.
```

```admonish failure title="Expected Test Failure"
Tests `require_self_panics`, `require_cycle_a_panics`, and `require_cycle_b_panics` will fail as expected, which we will fix in this section!
```

Run the tests with `cargo test`, or skip running them (and comment them out) if you don't want to waste battery life running infinite recursions.
These tests will infinitely recurse and thus fail.

The issue is that we only add a dependency to the dependency graph _after the task has executed_.
We do this because we need the output from the executing task to create the dependency.
Therefore, no dependencies are ever added to the dependency graph in these tests, because a task never finishes executing!
This in turn causes the cycle detection to never trigger, because there is no cycle in the dependency graph.

To fix this, we need to add task dependencies to the dependency graph _before we execute the task_.
But we cannot do this, because we need the output of the task to create the task dependency.
Therefore, we need to first _reserve_ a task dependency in the dependency graph, which creates an edge in the dependency graph without any attached data.

## Reserving task dependencies

2 changes: 1 addition & 1 deletion tutorial/src/SUMMARY.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
- [Fix Superfluous Task Dependency](./3_min_sound/4_fix_task_dep/index.md)
- [Prevent Overlapping File Writes](./3_min_sound/5_overlap/index.md)
- [Prevent Hidden Dependencies](./3_min_sound/6_hidden_dep/index.md)
- [Prevent Cycles]()
- [Prevent Cycles](./3_min_sound/7_cycle/index.md)
- [Improvements]()
- [Serialization]()
- [More Stampers]()
Expand Down
9 changes: 9 additions & 0 deletions tutorial/stepper/src/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -378,5 +378,14 @@ pub fn step_all(
SourceArchive::new("source.zip")
);
});

stepper.with_path("7_cycle", |stepper| {
stepper.apply([
create_diff_from_destination_file("a_task.rs", "../tests/common/mod.rs"),
]);
stepper.apply_failure([
add("b_test.rs", "../tests/top_down.rs"),
]);
});
});
}

0 comments on commit 0e3a302

Please sign in to comment.