test: implement safe abstraction using the Wake trait

[nurmohammed840]

No description provided.

[Darksonn]

This seems to contain two independent changes: using the Wake trait and a new block_on method. Could you explain what you're trying to do?

[nurmohammed840]

I'm really surprised that there isn't a block_on function:
Although original implementation do support this:

tokio/tokio-test/src/task.rs

Lines 209 to 212 in 9cc4a81

	ThreadWaker {
	state: Mutex::new(IDLE),
	condvar: Condvar::new(),
	}

tokio/tokio-test/src/task.rs

Lines 245 to 247 in 9cc4a81

	// The other half is sleeping, so we wake it up.
	assert_eq!(prev, SLEEP);
	self.condvar.notify_one();

Original implementation has Condvar, But it isn't used anywhere.
Also the name ThreadWaker implies that it's used to wake a sleeping thread, but no thread is actually sleeping (because there is no block_on), I believe the author may have forgotten to include block_on function.

This is my main reason to add block_on

It could also be useful as a basic future executor, similar to futures::executor::block_on

[Darksonn]

We use tokio_test::task::spawn quite a lot in our test suite, and we don't need it to include a block_on there. Usually, it's used to manually call poll in a convenient manner. What are you trying to use it for?

[nurmohammed840]

This could be useful outside of Tokio test suites for general-purpose future execution.
Most test cases don't require an I/O event loop or Timer

It's also well-suited for benchmarking asynchronous code due to its lightweight nature.

In some cases, you may want to avoid a runtime. For example, when running tools like Miri,
Currently #[tokio::test] cannot be used to test async code using miri.

---

Most importantly original implementation has already support for it.

Maybe we should consider replacing entire wake() function with just single AtomicBool::store operation, if we decide to drop block_on support.

tokio/tokio-test/src/task.rs

Line 230 in 161b8c8

fn wake(&self) {

[Darksonn]

Maybe we should consider replacing entire wake() function with just single AtomicBool::store operation, if we decide to drop block_on support.

Right now, Task<F> implements the Future trait and can be passed to tokio::runtime::Runtime::block_on. The change you suggest would break that, as wakeups would no longer be sent.

It's also well-suited for benchmarking asynchronous code due to its lightweight nature.

It can't benchmark anything other than pure computation. At that point, why is it async at all? Using async in benchmarks is fraught with issues and very difficult to do right.

More importantly, the purpose of tokio_test::task::spawn is not to be useful in benchmarks. Its purpose is to be useful for testing that async synchronization primitives send wakeups at the right times. Don't give it additional purposes.

This could be useful outside of Tokio test suites for general-purpose future execution.

Why should we make tokio_test::task::spawn be useful for things other than writing tests? Writing tests is the entire purpose of the tokio-test crate. This crate is the wrong place for general-purpose future execution. If people want that, they can create a Tokio runtime. If they don't want IO/timer drivers, they can create a Tokio runtime without IO/timer drivers. Or they can use futures::executor::block_on. Telling them to use the tokio-test crate in that case makes no sense.

---

As a more general concern, your PR makes two different changes:

• Use the Wake trait to get rid of unsafe.
• Add a block_on function.

I'm skeptical about adding a block_on, but even if we do that, these two independent changes should not be in the same PR.

[nurmohammed840]

It can't benchmark anything other than pure computation. why is it async at all ?

You're absolutely right, It's primarily intended for testing raw computation. Some library provide only async version (for simplicity
reasons) of the API. (Although those are just pure computation)

Why should we make tokio_test::task::spawn be useful for things other than writing tests?

Perhaps we don't need Spawn::block_on, I just thought it would be useful without adding additional dependency just to execute future.

I'm skeptical about adding a block_on

Let's remove that feature for now.