New project modal: Display logs + Cancel task#257
Conversation
a051cf8 to
9a754bd
Compare
| // Cooperative cancel: flip the flag the worker polls and | ||
| // keep the modal open showing "Cancelling…". The poller's | ||
| // Err(Cancelled) arm will switch the status to | ||
| // "Cancelled." once the worker unwinds (the worker kills | ||
| // the child + drainer threads join + final Err returns), | ||
| // leaving the user free to tweak inputs and retry. |
There was a problem hiding this comment.
while interesting, not sure implementation details are well served in comments, as it is it's describing the code..
| let mut cancelled = false; | ||
| let status = loop { | ||
| // Don't busy wait (try_wait is non-blocking). | ||
| std::thread::sleep(Duration::from_millis(50)); |
There was a problem hiding this comment.
https://doc.rust-lang.org/std/hint/fn.spin_loop.html might be a better fit or another way of awaiting/polling without any blocking but I dont think it's a big deal if it works and doesn't potentially deadlock.
There was a problem hiding this comment.
I don't think spin_loop is particularly better than a sleep here as we expect the wait to take some time anyway.
That being said, I can see sleep being a problem if we target web ? Any insight about that ?
There was a problem hiding this comment.
I dont think it will deadlock on web currently but it might lag. I'm not sure how realistic targeting web is at this stage so not sure we can try it out at this time.
There was a problem hiding this comment.
I don't think spin_loop is particularly better than a sleep here as we expect the wait to take some time anyway.
Fair enough, I agree if it doesn't cause issues its fine.
There was a problem hiding this comment.
You actually convinced me:
- sleep is probably blocking in single thread environment, where tasks have to run on the same thread
- not really an issue, but web wouldn't like us using sleep 😅
There was a problem hiding this comment.
i don't have plans to support jackdaw on web.....yet
| .unwrap_or_default(); | ||
| return Err(BuildError::Cancelled { stderr_tail: tail }); | ||
| } | ||
| std::thread::sleep(Duration::from_millis(50)); |
There was a problem hiding this comment.
let's keep the discussion on #257 (comment) then.
|
|
||
| // Scaffold. | ||
| if let Some(task) = state.scaffold_task.as_mut() | ||
| && let Some(result) = future::block_on(future::poll_once(task)) |
There was a problem hiding this comment.
Might be fine but block_on could also potentially cause deadlocks down the line if called from the main UI thread on Windows/MacOS while requiring the main thread inside the child somewhere (e.g. a popup file dialog)
There was a problem hiding this comment.
Here we use poll_once, so we're not really blocking here 🎉
9a754bd to
a866fa0
Compare
2bff2a3 to
a5ee4a4
Compare
0bb2aaa to
fa9960b
Compare
wip attempt on improving the new project modal:
possible follow ups