If we're only writing to the basic information field why are we querying the extended information?

Looks like this is a carry-over from the previous code... Might be a problem for another PR.

The actual logic in the referenced code is somewhat more sophisticated.

Specifically it handles CTRL_CLOSE_EVENT, CTRL_LOGOFF_EVENT, and CTRL_SHUTDOWN_EVENT specially by disabling JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE. The reason here is, as I understand it, ignoring these events doesn't stop windows from killing your process, and these events are also going to go to the child anyway. Disabling KILL_ON_JOB_CLOSE gives the child a chance to more gracefully handle these.

Although, again, this looks like it was taken from the previous code, so maybe not relevant here.

If we do this we might want to drop the job first too.

It's unclear to me if we need to do either drops manually? 🤔

I don't believe we need to drop child manually at all.

But I did think about this a bit further and I think there's another subtle problem. Job might get dropped immediately after we assign the process as we never use it again.

I think dropping job here explicitly might guarantee it lives long enough... There are also some other options.

But, I think really what we should do here is something like:

struct SupervisedChild<'a> {
    job: HANDLE,
    child: HANDLE,
    _child_lifetime: PhantomData<&'a ()>,
}

impl<'a> SupervisedChild<'a> {
    fn new<T>(child: HANDLE, _child_reference: &'a T) -> Self {
        let job = unsafe { CreateJobObjectW(None, None) }.map_err(|e| JobError::Create(e.code().0))?;
        // Configure limits
        // Assign the child
        // Maybe even set up the control handler given both code paths do it?
        Ok(Self { job, child, PhantomData::default() })
    }

    fn wait(self) -> Result<whatever> {
        WaitForSingleObject ...
    }
}

The PhantomData would ensure that the child outlives the JobSupervisedChild. And moving the wait here would ensure that we don't drop the SupervisedChild too early.

For the trampoline case we might just want to have a tiny wrapper around HANDLE that drops it at the right time and lets you access the HANDLE, then we can use a reference to the wrapper for the lifetime handling.

We could even go a step further and have the ChildHandle wrapper be in the same place as SupervisedChild, give it a From implementation for the std case, and then you can just give SupervisedChild the ChildHandle reference directly, let it keep it, and avoid the PhantomData.

One last alternative (probably too much boilerplate for two code paths): we could make SupervisedChild generic over the type, have our ChildHandle wrapper for the trampoline case, have it implement our own AsRawHandle trait, and then implement it for Child in std contexts...

Hm... https://doc.rust-lang.org/stable/std/os/windows/io/struct.BorrowedHandle.html

spawn.rs is std only right now.

We may want to fix this in the trampolines too but maybe I'll just do that in a separate change? It seems like it'd be a pre-existing problem?

I guess in the worst-case we just vendor the std impl?

The existing code (in the trampolines) didn't have this problem because it didn't have a Drop impl on the Job handle so it couldn't automatically get cleaned up too early.

Ah, I see. Will look at that briefly then.

Looks like I was mistaken, rust doesn't allow drop to be called earlier than the end of the scope.