New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Should not release GIL while running RegEnumValue #54280
Comments
Currently, PC/winreg.c releases GIL while calling registry http://msdn.microsoft.com/en-us/library/ms724865%28VS.85%29.aspx
Maybe we shouldn't release GIL in PC/winreg.c? Thank you. |
Some of your submission appears to have gotten lost (I saw it via email). # I sometimes experienced crash of test_changing_value(test_winreg) Index: PC/_winreg.c while (1) {
- Py_BEGIN_ALLOW_THREADS
rc = RegEnumValue(hKey,
index,
retValueBuf,
@@ -1228,7 +1227,6 @@
&typ,
(BYTE *)retDataBuf,
&retDataSize);
- Py_END_ALLOW_THREADS
if (rc != ERROR_MORE_DATA)
break;
@@ -1577,9 +1575,7 @@
if (subKey == NULL)
return NULL;
}
- Py_BEGIN_ALLOW_THREADS
rc = RegSetValue(hKey, subKey, REG_SZ, str, len+1);
- Py_END_ALLOW_THREADS
if (rc != ERROR_SUCCESS)
return PyErr_SetFromWindowsErrWithFunction(rc, "RegSetValue");
Py_INCREF(Py_None); |
Hirokazu, could you run Python in a debugger and figure out exactly which line crashes and what the error is? I'm curious. If we make this change, the same change should be applied to RegEnumKey, etc., since the RegEnumKey docs contain similar language. |
One thread is exactly on RegEnumValue |
LGTM I suggest that you add a comment with a link to this issue. People may wonder why some places don't release the GIL. |
Apart from the request for a comment made in msg192649 it looks as if this can be commited. |
I don't think this is an appropriate fix, since in most cases there is no need to prevent other Python threads running while inside RegSetValue. There are also other ways that a context switch may occur during the enumeration which will put the program in exactly the same state. (It isn't clear from the patch, but the two sections of changed code are from completely different functions.) The correct fix should be at the user's application level, or in a higher-level module than _winreg is supposed to be. |
With Steve's opposition and the fact that we've gotten along for 11 years since this issue was opened without it (and also without further reports of issues), I'm going to go ahead and close the issue. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: