Correct threading model used by wmi calls

[stevenh]

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.

[ahall]

I am using this in production now along with the Enum changes. Who can we get to merge these pull requests? Pretty messy having to apply them manually. Is @mjibson no longer maintaining this? :).

[maddyblue]

I'm not maintaining this anymore. @gbrayut may be, or may know who should look at these PRs.

[gbrayut]

I staged these at https://github.com/StackExchange/wmi/tree/next and will do some testing before merging to master

[gbrayut]

Holding off on merging... appears to be a memory leak. See https://github.com/multiplay/wmi/pull/8#issuecomment-235421732

[gbrayut]

Merged in https://github.com/StackExchange/wmi/pull/16 but we kept the lock and ole.CoInitializeEx(0, ole.COINIT_MULTITHREADED) as ole.CoInitializeEx(0, ole.COINIT_APARTMENTTHREADED) caused a memory leak

[gbrayut]

Hmm... still seeing a memory leak, although much slower than the apartment threaded one:

I am going to try reverting the ole.CoUninitialize() logic back into an else block. We may not be initializing things correctly (this says it is only needed once per thread and SHOULD have matching calls to CoUninitialize) but I'm not sure how to do that when the method is called by many go routines.

We didn't see any issues with the old logic, and the nil check would remain to fix the potential panic reported on the multiplay repo.

[stevenh]

Hmm that does sound very broken upstream as the MS docs do explicitly state, as you know:

To close the COM library gracefully on a thread, each successful call to CoInitialize or CoInitializeEx, including any call that returns S_FALSE, must be balanced by a corresponding call to CoUninitialize.

With regards to balancing the calls that's not something you need to worry about as its uninitialized before the method returns by the defer so the calls are explicitly balanced and as the thread is locked for the duration that thread cant be used by any other executing goroutine for the duration.

Do you have a small re-production case you could share?

[gbrayut]

The code base is https://github.com/bosun-monitor/bosun/tree/master/cmd/scollector and https://github.com/bosun-monitor/bosun/blob/master/cmd/scollector/collectors/cpu_windows.go#L16 is an example of a collector that gets run every 15s. There are other collectors also creating WMI queries using 90-100 total go routines.

I suspect https://github.com/StackExchange/wmi/blob/master/wmi.go#L12 would have the same issue if it is run in a go benchmark, but I'm on day two of what I though would be a quick merge so I don't have time to test that.

So far testing with the old CoUninitialize code looks better, but I'll need to let it run for a few more hours to confirm:

[gbrayut]

I think I found it. It wasn't this PR, it was the Use _NewEnum one which was missing a call to Release(). https://github.com/StackExchange/wmi/pull/14#issuecomment-236031901

We will leave the new ole.CoUninitialize() code in, as testing that by itself did not cause any issues.

[stevenh]

Cool thanks for the update @gbrayut that would explain why we never saw an issue in our production environment either.

If adding that missing Release() does indeed fix it adding back in the switch to apartment threading would also be good to see :)

[gbrayut]

I let it run overnight with apartment threading and it looks slightly higher but no leak. I'll merge that in to master shortly

Thanks!