[Bug 38143] New: IO completions cause wineserver to leak APC packets
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(a)winehq.org Reporter: dmitry(a)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. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=38143 --- Comment #1 from Dmitry Timoshkov <dmitry(a)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. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=38143 Dmitry Timoshkov <dmitry(a)baikal.ru> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |download, patch, source, | |testcase -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=38143 --- Comment #2 from Bruno Jesus <00cpxxx(a)gmail.com> --- Reminds me of bug 37038. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=38143 --- Comment #3 from Dmitry Timoshkov <dmitry(a)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. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=38143 Sebastian Lackner <sebastian(a)fds-team.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |sebastian(a)fds-team.de --- Comment #4 from Sebastian Lackner <sebastian(a)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. ;) -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=38143 --- Comment #5 from Dmitry Timoshkov <dmitry(a)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 :) -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=38143 --- Comment #6 from Dmitry Timoshkov <dmitry(a)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. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=38143 --- Comment #7 from Alexandre Julliard <julliard(a)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. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=38143 --- Comment #8 from Dmitry Timoshkov <dmitry(a)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. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=38143 --- Comment #9 from Sebastian Lackner <sebastian(a)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. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=38143 --- Comment #10 from Dmitry Timoshkov <dmitry(a)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. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=38143 --- Comment #11 from Sebastian Lackner <sebastian(a)fds-team.de> --- I assume this is fixed with http://source.winehq.org/git/wine.git/commit/4273b0d938b9ed64c0edefdeb96cda3... Could you please test @ Dmitry? -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=38143 Dmitry Timoshkov <dmitry(a)baikal.ru> changed: What |Removed |Added ---------------------------------------------------------------------------- Fixed by SHA1| |4273b0d938b9ed64c0edefdeb96 | |cda33d1479bb8 Status|NEW |RESOLVED Resolution|--- |FIXED --- Comment #12 from Dmitry Timoshkov <dmitry(a)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. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=38143 Alexandre Julliard <julliard(a)winehq.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED --- Comment #13 from Alexandre Julliard <julliard(a)winehq.org> --- Closing bugs fixed in 1.7.38. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
participants (1)
-
wine-bugs＠winehq.org