Re: [PATCH 1/2] server: Store async list in process object and use that to find async in cansel_async request.
Jacek Caban <jacek(a)codeweavers.com> writes:
@@ -614,6 +615,7 @@ static void process_destroy( struct object *obj )
/* we can't have a thread remaining */ assert( list_empty( &process->thread_list )); + assert( list_empty( &process->asyncs ));
I don't think that this is guaranteed, I believe that there can still be asyncs in the queues of shared files. We'd probably need to clear them explicitly at process exit. -- Alexandre Julliard julliard(a)winehq.org
On 30.11.2016 16:27, Alexandre Julliard wrote:
Jacek Caban <jacek(a)codeweavers.com> writes:
@@ -614,6 +615,7 @@ static void process_destroy( struct object *obj )
/* we can't have a thread remaining */ assert( list_empty( &process->thread_list )); + assert( list_empty( &process->asyncs )); I don't think that this is guaranteed, I believe that there can still be asyncs in the queues of shared files.
I think it is guaranteed. async object holds thread reference and thread object holds process reference, so process object will not be destroyed until all its asyncs are released. Asyncs in queues of shared objects will keep process object alive (and they shouldn't), but it's not new with my patch. Am I missing something?
We'd probably need to clear them explicitly at process exit.
I agree that we should. I will send a new series implementing that. I tested that with the attached patch (I can't send it to wine-patches yet because it uses pipes in message mode). Thanks, Jacek
Jacek Caban <jacek(a)codeweavers.com> writes:
On 30.11.2016 16:27, Alexandre Julliard wrote:
Jacek Caban <jacek(a)codeweavers.com> writes:
@@ -614,6 +615,7 @@ static void process_destroy( struct object *obj )
/* we can't have a thread remaining */ assert( list_empty( &process->thread_list )); + assert( list_empty( &process->asyncs )); I don't think that this is guaranteed, I believe that there can still be asyncs in the queues of shared files.
I think it is guaranteed. async object holds thread reference and thread object holds process reference, so process object will not be destroyed until all its asyncs are released. Asyncs in queues of shared objects will keep process object alive (and they shouldn't), but it's not new with my patch. Am I missing something?
No, you're right, it's holding a thread reference so it's safe. This is of course a leak, but it's not a new issue.
We'd probably need to clear them explicitly at process exit.
I agree that we should. I will send a new series implementing that. I tested that with the attached patch (I can't send it to wine-patches yet because it uses pipes in message mode).
Thread exit would be interesting to test too. -- Alexandre Julliard julliard(a)winehq.org
On 01.12.2016 12:52, Alexandre Julliard wrote:
Jacek Caban <jacek(a)codeweavers.com> writes:
We'd probably need to clear them explicitly at process exit. I agree that we should. I will send a new series implementing that. I tested that with the attached patch (I can't send it to wine-patches yet because it uses pipes in message mode). Thread exit would be interesting to test too.
I tested that and it seems that Windows aborts asyncs on thread exit. It's easy to implement aborting itself in Wine, but I must admit that I have very mixed feelings about it. I would expect to be able to initialize an async I/O in one thread and handle its completion in another. That's how threadpool - based applications are likely to work and, maybe not literally, but [1] gives an impression that it's supported. I tried to test threadpool with the attached test, but it throws an exception in TerminateThread call, not allowing me to terminate that thread. That makes me think that it supports splitting async I/O between threads by heavily ensuring that affected threads are not terminated. We don't have something like that in Wine and I find it very risky to implement async cancellation on thread exit at this point. Thanks, Jacek [1] https://msdn.microsoft.com/en-us/library/windows/desktop/ms686760(v=vs.85).a...
Jacek Caban <jacek(a)codeweavers.com> writes:
On 01.12.2016 12:52, Alexandre Julliard wrote:
Jacek Caban <jacek(a)codeweavers.com> writes:
We'd probably need to clear them explicitly at process exit. I agree that we should. I will send a new series implementing that. I tested that with the attached patch (I can't send it to wine-patches yet because it uses pipes in message mode). Thread exit would be interesting to test too.
I tested that and it seems that Windows aborts asyncs on thread exit. It's easy to implement aborting itself in Wine, but I must admit that I have very mixed feelings about it.
I would expect to be able to initialize an async I/O in one thread and handle its completion in another. That's how threadpool - based applications are likely to work and, maybe not literally, but [1] gives an impression that it's supported. I tried to test threadpool with the attached test, but it throws an exception in TerminateThread call, not allowing me to terminate that thread. That makes me think that it supports splitting async I/O between threads by heavily ensuring that affected threads are not terminated. We don't have something like that in Wine and I find it very risky to implement async cancellation on thread exit at this point.
It looks like it would need more work, yes. I guess for now, freeing them at process exit is still better than not freeing them at all ;-) Thanks for checking this! -- Alexandre Julliard julliard(a)winehq.org
participants (2)
-
Alexandre Julliard -
Jacek Caban