Refactored the TLS use of the new runtime #8116
Conversation
|
Please go through all the calls to schedule_task and any others that now may return the Scheduler and make sure they do something with it. I've run into a few cases where this error resulted in the scheduler being destroyed erroneously. |
|
That may be a good indication that this is the wrong pattern to use for these methods - it makes the callers error-prone. |
|
I haven't yet audited the uses, but I think it boils down to a task versus scheduler context issue. While in scheduler context it works smoothly, but schedule_task is primarily for task context where we don't want to deal with this. Modifying task-context methods to use a different pattern sounds good to me, I'll ponder what the best design there is. Composing schedule_task with a wrapper that properly handles the returned value seems nice but might end up crude without good function composition operators. I think what I intended is that schedule_task is just a "piece" that is used to build higher level functions that deal with the return value, but that vision might have been polluted. I know schedule_task has "more" logic than it should currently, it was much more complex than it should have to parameterize it over context. (the copy-paste commit) |
|
Attempted to reproduce locally, didn't succeed. We should talk about this in the morning. |
|
Looking into the failed test more. It appears to be failing as an old runtime test which is extremely odd, as that code shouldn't have changed at all. The tricky bit seems to be sending to a SharedChan inside a destructor. |
old design the TLS held the scheduler struct, and the scheduler struct held the active task. This posed all sorts of weird problems due to how we wanted to use the contents of TLS. The cleaner approach is to leave the active task in TLS and have the task hold the scheduler. To make this work out the scheduler has to run inside a regular task, and then once that is the case the context switching code is massively simplified, as instead of three possible paths there is only one. The logical flow is also easier to follow, as the scheduler struct acts somewhat like a "token" indicating what is active. These changes also necessitated changing a large number of runtime tests, and rewriting most of the runtime testing helpers. Polish level is "low", as I will very soon start on more scheduler changes that will require wiping the polish off. That being said there should be sufficient comments around anything complex to make this entirely respectable as a standalone commit.
…ng RUST_NEWRT to test.
… it would "bounch" a regular task in and out of the work queue without allowing a different scheduler to run it.
…ter inside the task struct, and also added an assert to verify that send is never called inside scheduler context as it is undefined (BROKEN) if that happens
…scheduler functions slightly
…ask> so that the log methods will work
Merged with task killing code this time around.
Merged with task killing code this time around.