A partial fix for issue #1191. Do not panic mio on sockstate error, i…

[dtacalau]

…nstead keep the err sock states and just retry them later.

This is a partial fix for 0.7 because it just handles the errors, and makes the sockstates available next poll instead of panicking. Pushing the error to user is more complicated as it would require new api added. I'd suggest to leave this part until custom IOCP handlers are implemented and #1195 is merged.

[Thomasdezeeuw]

I haven't been able to find any documentation on NtCancelIoFileEx (the system call made here), do you have a documentation @dtacalau?

I want to know what kind of errors can be returned here because maybe we can ignore some of them.

[dtacalau]

@Thomasdezeeuw not much, seems it can return any NTSTATUS. The only I was able to found:

• some of these unit tests check result of NtCancelIoFileEx https://git.reactos.org/?p=reactos.git;a=blob;hb=6d0c07c44fae0a93c57f8d3410545e0f3c201287;f=modules/rostests/winetests/ntdll/pipe.c#l384
• Windows list of NTSTATUS codes https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-erref/596a1078-e883-4972-9bbc-49e60bebca55

[Thomasdezeeuw]

Always great when you're developing against an API for which you don't have docs, right?

[dtacalau]

rhetorical question I guess :)

[kleimkuhler]

Overall looks good. I'd like some of these comments addressed, but should be a 👍 afterwards.

[kleimkuhler]

Sorry missed something in my first review.

[dtacalau]

I've done a change in the logic of update_sockets_events. The following block should be done no matter if a sock update failed or not, because if we do it only on error then we end up with duplicates in the update_queue:

        update_queue.retain(|sock| {
            sock.lock().unwrap().has_error()
        });

I've also found another issue we need to address: reregister done on a sock which has_error will result in having the same socked added twice in the update queue. Will not result in a bug, just that we'll call update twice, 2nd call being useless. To fix this self.add_socket_to_update_queue(socket); has to be called only if sock.has_error is false.
@Thomasdezeeuw any idea how can I achieve this? I got into mutability issues, cannot move out of a lock, so I cannot check for has_error and add it to the queue at the while sock.lock is being held.

[kleimkuhler]

@dtacalau I still need to familiarize more with the Windows impl, but could set_event do the check and return false if SockState.error is true? We hold the lock right before going into that block of code so it could just return early

[kleimkuhler]

@dtacalau Also I would appreciate it for changes in general if you can not force push commits 🙂. It makes it difficult to review PRs. I like to be able to leave a review, and then when a review is re-requested I can see what changed since the last time.

When a force push happens, the "viewed already" cache resets and I don't have a way to tell what is new and what I've reviewed.

I wouldn't worry about the git log either because commits should be squashed in a PR.

[dtacalau]

@dtacalau I still need to familiarize more with the Windows impl, but could set_event do the check and return false if SockState.error is true? We hold the lock right before going into that block of code so it could just return early

@kleimkuhler Yeah that would work, By changing set_event to return has_error we avoid double locking. Another way would be to check has_error under lock, like this sock.lock().unwrap().has_error():

if (sock.lock().unwrap().set_event(event)) {
                self.add_socket_to_update_queue(socket);
}
self.update_sockets_events_if_polling()?;

[dtacalau]

@dtacalau Also I would appreciate it for changes in general if you can not force push commits .

Ok, got it, sorry for that.

[kleimkuhler]

@dtacalau Yea so we still want to call socket.set_sock_state(Some(sock)); after set_event it looks like. In that case, set_event could return false if error.is_some(), and then we only update based on that result. So something like:

let should_update = sock.lock().unwrap().set_event(event);
socket.set_sock_state(Some(sock));
if should_update {
    unsafe {
        self.add_socket_to_update_queue(socket);
        self.update_sockets_events_if_polling()?;
    }
}

Edit: That way, we aren't locking it multiple times

[dtacalau]

rebased on master

@kleimkuhler I'm not quite happy with the solution from #1215 (comment). It works but it feels like a hack conveniently modifying the return of set_event for this fix. What if we need it to return something else for another issue we might find :)?

I'm going to leave it as it is until I figure something cleaner. WDYT?

[Thomasdezeeuw]

Thanks @dtacalau, sorry for the late review.