Correct threading model used by wmi calls #8
Conversation
WbemScripting is apartment threaded according to its registry entries so switch multi-threaded OLE to apartment threaded enabling the global lock to be removed. Correct missing CoUninitialize call when CoInitializeEx returns S_FALSE as documented here: https://msdn.microsoft.com/en-us/library/windows/desktop/ms695279(v=vs.85).aspx Also: * Cleanup OleError checking in Query. * Handle nil return from CreateObject by returning ErrNilCreateObject before it causes a panic. This has been seen in rare cases in production when built under go v1.6.
|
👍 |
1 similar comment
|
👍 |
|
We're seeing a memory leak in scollector after applying this change. Previously it use to consume ~50MB, and now it grows to over 500MB after a few hours and over 1GB on other systems:
You can see the test branch that has the commit from this PR and another on PR. We ran test builds with our previous master, both this commit and the NewEnum commit, and just this commit, and narrowed down the leak to somewhere in this diff. Have you seen any memory issues with this? |
|
Can't say we saw that while we had this in production. I would have expected it if anything to be the opposite, given the previously missing CoUninitialize. What OS are you using as WMI is quite known for having leaks, requiring OS patches in the past? One thing I would That said we've had to eliminate our use of WMI due to very poor performance in comparison to direct native calls for the workload we were using it for (obtaining process and machine stats). |
|
We saw the issue on both Windows 2012 R2 and Server 2008 R2 with latest patches. We run scollector on all systems as a monitoring agent and it uses WMI to pull a number of performance counters on 15s intervals. We've seen our share of issues with WMI, but the increased memory usage was only seen as part of a canary build when trying to merge this PR into the upstream repo. We're digging thru the pprof output to try and find where the leak is, but if we can't find it we likely will just exclude this PR from the merge as we don't have a lot of WMI/OLE expertise on our team right now. |
This reverts part of commit 04850ec. using COINIT_APARTMENTTHREADED caused a memory leak. See Unity-Technologies#8
|
So it looks like just switching to COINIT_APARTMENTTHREADED and removing the lock and is enough to cause the leak. My guess is that because we call the Query function from multiple go routines which may cycle thru various OS threads during the lifetime of the program, it is generating a lot of new apartments that somehow aren't being cleaned up correctly. The docs say that the first "call to CoInitializeEx(NULL, COINIT_MULTITHREADED) creates a multi-threaded apartment that will be shared with any other threads that repeat this call", which likely keeps things from leaking on the OLE/COM side (pprof wasn't much help and didn't see any leaking objects). We've never had any issues with COINIT_MULTITHREADED, so I'm going to stick with that and merge the other changes so long as it survives a stress test tomorrow. I see in go-ole/go-ole#122 you had panics when you upgraded to go 1.6.2, but so far we haven't seen those issues. |
|
Yes the check and ErrNilCreateObject return should protect against the panic on 1.6.2. It sounds like COINIT_APARTMENTTHREADED creating a per thread apartment is indeed causing a leak in the in the provider. Searching in the web there seem to be a number of reports of this, strange that we never saw it though |
WbemScripting is apartment threaded according to its registry entries so switch multi-threaded OLE to apartment threaded enabling the global lock to be removed.
Correct missing CoUninitialize call when CoInitializeEx returns S_FALSE as documented here:
https://msdn.microsoft.com/en-us/library/windows/desktop/ms695279(v=vs.85).aspx
Also: