https://bugs.winehq.org/show_bug.cgi?id=38143
Bug ID: 38143 Summary: IO completions cause wineserver to leak APC packets Product: Wine Version: 1.7.37 Hardware: x86 OS: Linux Status: NEW Severity: normal Priority: P2 Component: -unknown Assignee: wine-bugs@winehq.org Reporter: dmitry@baikal.ru Distribution: ---
Created attachment 50870 --> https://bugs.winehq.org/attachment.cgi?id=50870 leaky io ports application
This started as a report that after long time execution (couple of days) wineserver memory usage grows by tens hundreds of megabytes.
Attached is a simple .net application (with source) that demonstrates the problem. Steps to reproduce: 1. rm -rf ~/.wine 2. winetricks -q dotnet40 3. wine IOPortsLeakyApp.exe
And in a separate terminal watch how wineserver heap usage starts to very quickly grow: pmap -X -p `pidof wineserver` | grep heap
The source of the leak is at server/async.c,async_set_result(), a queued thread APC packet is never fetched from the queue and as a result is never executed and freed.
The problem is that an APC packet is added to queue as APC_USER type and that needs an alterable thread wait, otherwise server/thread.c,select() handler (actually thread_dequeue_apc(), but that's minor detail) will fetch only system APCs and never user ones.
One solution would be to put a thread into alertable wait so it has an opportunity to handle user APCs, another is to add an IO completion APC to the system instead of user APC queue.
https://bugs.winehq.org/show_bug.cgi?id=38143
--- Comment #1 from Dmitry Timoshkov dmitry@baikal.ru --- Created attachment 50871 --> https://bugs.winehq.org/attachment.cgi?id=50871 patch
Attached patch puts a thread into alertable state so that NtRemoveIoCompletion is actually able to fetch a waiting APC from the queue.
https://bugs.winehq.org/show_bug.cgi?id=38143
Dmitry Timoshkov dmitry@baikal.ru changed:
What |Removed |Added ---------------------------------------------------------------------------- Keywords| |download, patch, source, | |testcase
https://bugs.winehq.org/show_bug.cgi?id=38143
--- Comment #2 from Bruno Jesus 00cpxxx@gmail.com --- Reminds me of bug 37038.
https://bugs.winehq.org/show_bug.cgi?id=38143
--- Comment #3 from Dmitry Timoshkov dmitry@baikal.ru --- (In reply to Bruno Jesus from comment #2)
Reminds me of bug 37038.
Sounds like the same issue, and the bug 36662 referenced from there.
https://bugs.winehq.org/show_bug.cgi?id=38143
Sebastian Lackner sebastian@fds-team.de changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |sebastian@fds-team.de
--- Comment #4 from Sebastian Lackner sebastian@fds-team.de --- (In reply to Dmitry Timoshkov from comment #1)
Created attachment 50871 [details] patch
Attached patch puts a thread into alertable state so that NtRemoveIoCompletion is actually able to fetch a waiting APC from the queue.
Does that match the Windows behaviour, or is it just a hack? Since applications can also queue their own APCs (with QueueUserAPC) Wine should try to stay as close as possible to the Windows behaviour. Moreover, please note that a wait is interrupted when APC calls are handled, so with your patch applied NtRemoveIoCompletion returns a different status value in some situations. It sounds like this might need some additional tests. ;)
https://bugs.winehq.org/show_bug.cgi?id=38143
--- Comment #5 from Dmitry Timoshkov dmitry@baikal.ru --- (In reply to Sebastian Lackner from comment #4)
Attached patch puts a thread into alertable state so that NtRemoveIoCompletion is actually able to fetch a waiting APC from the queue.
Does that match the Windows behaviour, or is it just a hack? Since applications can also queue their own APCs (with QueueUserAPC) Wine should try to stay as close as possible to the Windows behaviour. Moreover, please note that a wait is interrupted when APC calls are handled, so with your patch applied NtRemoveIoCompletion returns a different status value in some situations. It sounds like this might need some additional tests. ;)
That's the point of a bug report - discuss the problem and find a proper solution :)
https://bugs.winehq.org/show_bug.cgi?id=38143
--- Comment #6 from Dmitry Timoshkov dmitry@baikal.ru --- (In reply to Sebastian Lackner from comment #4)
Does that match the Windows behaviour, or is it just a hack? Since applications can also queue their own APCs (with QueueUserAPC) Wine should try to stay as close as possible to the Windows behaviour. Moreover, please note that a wait is interrupted when APC calls are handled, so with your patch applied NtRemoveIoCompletion returns a different status value in some situations. It sounds like this might need some additional tests. ;)
The tests I've added clearly show that user APCs are not supposed to be executed during GetQueuedCompletionStatus call, so my patch is wrong in that regard. On the other hand it looks like the queued APCs are never executed (and nothing in the attached application breaks) means that adding an APC to the queue as a reply to APC_ASYNC_IO request and actually executing it wasn't tested very well (if tested at all). So it raises a question whether APCs should be always generated by server, or generated at all.
https://bugs.winehq.org/show_bug.cgi?id=38143
--- Comment #7 from Alexandre Julliard julliard@winehq.org --- (In reply to Dmitry Timoshkov from comment #6)
The tests I've added clearly show that user APCs are not supposed to be executed during GetQueuedCompletionStatus call, so my patch is wrong in that regard. On the other hand it looks like the queued APCs are never executed (and nothing in the attached application breaks) means that adding an APC to the queue as a reply to APC_ASYNC_IO request and actually executing it wasn't tested very well (if tested at all). So it raises a question whether APCs should be always generated by server, or generated at all.
To be fully compatible it should of course only generate system APCs, but we can't do that since we want to call heap functions and we can't do that asynchronously.
https://bugs.winehq.org/show_bug.cgi?id=38143
--- Comment #8 from Dmitry Timoshkov dmitry@baikal.ru --- (In reply to Alexandre Julliard from comment #7)
The tests I've added clearly show that user APCs are not supposed to be executed during GetQueuedCompletionStatus call, so my patch is wrong in that regard. On the other hand it looks like the queued APCs are never executed (and nothing in the attached application breaks) means that adding an APC to the queue as a reply to APC_ASYNC_IO request and actually executing it wasn't tested very well (if tested at all). So it raises a question whether APCs should be always generated by server, or generated at all.
To be fully compatible it should of course only generate system APCs, but we can't do that since we want to call heap functions and we can't do that asynchronously.
Alexandre, what would you suggest as possible solution(s)?
Also, could you please add a comment with possible solutions/suggestions to the bug 37669 and reply to Sebastian's attempt to introduce limited support (http://source.winehq.org/patches/data/109331) for handling write watch exceptions on signal stack? That patch (as usually) is marked as pending without a slight insight what should be changed or reworked to get this fixed properly.
Both of this problems are caused by fundamental limitations in Wine and need IMHO help from your side. Thanks in advance.
https://bugs.winehq.org/show_bug.cgi?id=38143
--- Comment #9 from Sebastian Lackner sebastian@fds-team.de --- I assume the easiest way to get rid of the HeapFree user callback would be something similar to http://source.winehq.org/git/wine.git/patch/577cb166d48e5c6f270237179cdbeb82.... Most likely Alexandre is already working on it, if not I'll write a patch during the next days.
https://bugs.winehq.org/show_bug.cgi?id=38143
--- Comment #10 from Dmitry Timoshkov dmitry@baikal.ru --- (In reply to Sebastian Lackner from comment #9)
I assume the easiest way to get rid of the HeapFree user callback would be something similar to http://source.winehq.org/git/wine.git/patch/ 577cb166d48e5c6f270237179cdbeb82a68b1ac0. Most likely Alexandre is already working on it, if not I'll write a patch during the next days.
I guess there is no way to avoid duplicating this approach for every dll that implements async IO. Something like this should be done for winsock as well, and will avoid creating an APC just for the purpose of freeing some memory.
https://bugs.winehq.org/show_bug.cgi?id=38143
--- Comment #11 from Sebastian Lackner sebastian@fds-team.de --- I assume this is fixed with http://source.winehq.org/git/wine.git/commit/4273b0d938b9ed64c0edefdeb96cda3...
Could you please test @ Dmitry?
https://bugs.winehq.org/show_bug.cgi?id=38143
Dmitry Timoshkov dmitry@baikal.ru changed:
What |Removed |Added ---------------------------------------------------------------------------- Fixed by SHA1| |4273b0d938b9ed64c0edefdeb96 | |cda33d1479bb8 Status|NEW |RESOLVED Resolution|--- |FIXED
--- Comment #12 from Dmitry Timoshkov dmitry@baikal.ru --- (In reply to Sebastian Lackner from comment #11)
I assume this is fixed with http://source.winehq.org/git/wine.git/commit/ 4273b0d938b9ed64c0edefdeb96cda33d1479bb8
Could you please test @ Dmitry?
It's fixed now. Many thanks Alexandre.
https://bugs.winehq.org/show_bug.cgi?id=38143
Alexandre Julliard julliard@winehq.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED
--- Comment #13 from Alexandre Julliard julliard@winehq.org --- Closing bugs fixed in 1.7.38.
-
wine-bugs＠winehq.org