alamb commented on code in PR #15653:
URL: https://github.com/apache/datafusion/pull/15653#discussion_r2036167289
##########
datafusion/common-runtime/src/common.rs:
##########
@@ -77,17 +82,32 @@ impl<R: 'static> SpawnedTask<R> {
}
}
+impl<R> Future for SpawnedTask<R> {
+ type Output = Result<R, JoinError>;
+
+ fn poll(mut self: Pin<&mut Self>, cx: &mut Context<'_>) ->
Poll<Self::Output> {
+ Pin::new(&mut self.inner).poll(cx)
+ }
+}
+
+impl<R> Drop for SpawnedTask<R> {
+ fn drop(&mut self) {
+ self.inner.abort();
+ }
+}
+
#[cfg(test)]
mod tests {
use super::*;
use std::future::{pending, Pending};
- use tokio::runtime::Runtime;
+ use tokio::{runtime::Runtime, sync::oneshot};
#[tokio::test]
async fn runtime_shutdown() {
let rt = Runtime::new().unwrap();
+ #[allow(clippy::async_yields_async)]
Review Comment:
I think this is ok -- the only reason clippy picks this up now is that
`SpawnedTask` is actually a future where previously one had to call
`join().await` -- so TLDR this change makes sense to me (though the test is
perhaps somewhat suspect)
##########
datafusion/common-runtime/src/common.rs:
##########
@@ -47,22 +54,20 @@ impl<R: 'static> SpawnedTask<R> {
T: Send + 'static,
R: Send,
{
- let mut inner = JoinSet::new();
- inner.spawn_blocking(task);
+ #[allow(clippy::disallowed_methods)]
Review Comment:
```suggestion
// Ok to use spawn here as SpawnedTask handles aborting/cancelling
the task on Drop
#[allow(clippy::disallowed_methods)]
```
##########
datafusion/common-runtime/src/common.rs:
##########
@@ -36,8 +43,8 @@ impl<R: 'static> SpawnedTask<R> {
T: Send + 'static,
R: Send,
{
- let mut inner = JoinSet::new();
- inner.spawn(task);
+ #[allow(clippy::disallowed_methods)]
+ let inner = tokio::task::spawn(trace_future(task));
Review Comment:
I think some rationale about why it is ok to use the disallowed methods
would help. Something like:
```suggestion
// Ok to use spawn here as SpawnedTask handles aborting/cancelling
the task on Drop
let inner = tokio::task::spawn(trace_future(task));
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
