Re: [PATCH 1/2] server: Store async list in process object and use that to find async in cansel_async request.

[Alexandre Julliard]

Jacek Caban  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.

[PATCH 1/2] server: Store async list in process object and use that to find async in cansel_async request.

[Jacek Caban]

On 30.11.2016 16:27, Alexandre Julliard wrote:
> Jacek Caban  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

[PATCH 1/2] server: Store async list in process object and use that to find async in cansel_async request.

[Alexandre Julliard]

Jacek Caban  writes:
> On 30.11.2016 16:27, Alexandre Julliard wrote:
> > Jacek Caban  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.

[PATCH 1/2] server: Store async list in process object and use that to find async in cansel_async request.

[Jacek Caban]

On 01.12.2016 12:52, Alexandre Julliard wrote:
> Jacek Caban  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...

[PATCH 1/2] server: Store async list in process object and use that to find async in cansel_async request.

[Alexandre Julliard]

Jacek Caban  writes:
> On 01.12.2016 12:52, Alexandre Julliard wrote:
> > Jacek Caban  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!