Last error wrongly set by some modules

[CristiFati]

This is a proposed fix for #2301.

When searching for functions in libraries, GetModuleHandle is attempted 1^st (most likely to avoid incrementing RefCount), and only if it fails LoadLibrary is used. Unfortunately, for libraries that aren't already loaded ("secur32* for win32api or msimg32 for win32gui) GetModuleHandle sets last error flag to 126: ERROR_MOD_NOT_FOUND. Since the flag is not cleared, for win32gui my EnumWindows changes seem to have no effect.

The proposed fix is in a form of a macro that does the library loading (following Py_BEGIN_ALLOW_THREADS model) and clears this particular error.

Notes:

• A simpler way would be to simply clear the error once (just before module initialization end), but that's lame, and may swallow other errors that could come and bite in the ass at a later time (just like this one did)

• I only fixed these 2 modules (as a POC). I will fix the others affected (a quick code search revealed them), once I have the green light, as this is basically monkey-work (that I don't want to do more than absolutely necessary)

• My win32gui tests were not affected by it since I'm manually clearing the error flag. I remember that I ran into this error at the time, and clearing the flag fixed the problem which was good enough (I didn't dig deeper). But tests are perfectly fine, as they should start from a known (and deterministic) state

• I don't know why this method with function pointers was chosen (instead of linking with the .lib files), as a vast majority of these functions are there since Windows 2000 (and exposed by VStudio)

• I wouldn't be surprised if there were more open issues caused by this

[mhammond]

Can't we use a function?

[CristiFati]

It has been some time since I started this. Are the changes OK, is it safe to propagate them to other modules that have the same problem?

[mhammond]

Can't we use a function?

What I meant was that in this patch it seems that the new macro isn't really necessary and an (inline?) function seems cleaner. Is there a reason we can't do that? I'm not against the macro where it makes sense, but I don't see how it does in those 2 files.

[mhammond]

That looks great, thanks! How do you feel about using the name PyWin_GetOrLoadLibraryHandle or similar? Sorry to nit-pick and let me know if you feel strongly about any part of this!

[CristiFati]

I remembered why I used a macro: to avoid a direct call to *A functions and use the VStudio macros (e.g.: GetModuleHandle) instead.
Sure, will use PyWin_GetOrLoadLibraryHandle.
So, are the changes OK? Should I go ahead and do the rest of the modules?

[mhammond]

So, are the changes OK? Should I go ahead and do the rest of the modules?

The changes seem fine - if you update the name and remove that stray clang comment, I'm fine with landing this now and doing the rest in another PR, or extending this PR for the rest - up to you!

[mhammond]

thanks - seeing this is a bug fix, could you please add a changes entry?

[CristiFati]

It was the (dumb) formatter again.

Apparently, the [no ci] tag cancelled the CI workflows even for the previous commit (maybe because it's a rebase / force push?).
Anyway in order to follow the process they should run.

Unfortunately I don't know how (if) could I do it manually (even in my fork).

[Avasam]

@CristiFati You can normally retrigger GitHub workflows by closing/reopening a PR (that's what I do for random test failure and what we have to do for some automated PRs in Typeshed).

If you amend your last commit's name and force-push that should do it too.

[mhammond]

Thanks!