| msg66313 - (view) |
Author: Eric V. Smith (eric.smith) *  |
Date: 2008-05-06 14:14 |
| Per http://mail.python.org/pipermail/python-3000/2008-April/013094.html, add a PendingDeprecationWarning to 3.0 for % formatting. The attached patch implements this for 3.0. For 2.6, it should only be a PendingDeprecationWarning if -3 warnings are turned on. I'll work on that after the 3.0 patch is accepted. I'm not wild about using a global variable to disallow recursion, but it's probably the way to go. Question: Do I need to acquire the GIL here? Another issue is that the interpreter should probably at least start up without triggering these warnings. I'll add an issue for that at a later date. |
|
|
| msg66314 - (view) |
Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * |
Date: 2008-05-06 14:48 |
| > Do I need to acquire the GIL here? No, the GIL has already been acquired. The problem with the static variable is that while the main thread is PyErr_WarnEx(), another thread may gain focus. And it will not trigger the warning. |
|
|
| msg66316 - (view) |
Author: Eric V. Smith (eric.smith) * |
Date: 2008-05-06 14:55 |
| Right. But is it worth adding per-thread machinery to this? I was going to do that, but it seemed like overkill. Upon further reflection, however, I think you may be right. I'll remove the "easy" keyword! |
|
|
| msg66333 - (view) |
Author: Benjamin Peterson (benjamin.peterson) * |
Date: 2008-05-06 21:16 |
| Would it be practical to add a _PyErr_InErrorProcessing function to prevent recursion? |
|
|
| msg66337 - (view) |
Author: Eric V. Smith (eric.smith) * |
Date: 2008-05-06 21:50 |
| Since we're just trying to prevent this function from recursing (indirectly) into itself, I think all of the logic can go here. What would you suggest the function _PyErr_InErrorProcessing do differently? I think the real issue is: Does the additional logic and execution time involved in adding per-thread state justify being "more correct", or can we occasionally lose a warning message? |
|
|
| msg66338 - (view) |
Author: Benjamin Peterson (benjamin.peterson) * |
Date: 2008-05-06 21:58 |
| On Tue, May 6, 2008 at 4:50 PM, Eric Smith <report@bugs.python.org> wrote: > > Eric Smith <eric@trueblade.com> added the comment: > > Since we're just trying to prevent this function from recursing > (indirectly) into itself, I think all of the logic can go here. > > What would you suggest the function _PyErr_InErrorProcessing do differently? I was just thinking that this problem would probably come up again. > I think the real issue is: Does the additional logic and execution time > involved in adding per-thread state justify being "more correct", or can > we occasionally lose a warning message? Well, the first thing to check for is Py_Py3kWarning. Then do the extra logic and execution speed. |
|
|
| msg66346 - (view) |
Author: Eric V. Smith (eric.smith) * |
Date: 2008-05-06 23:36 |
| > Well, the first thing to check for is Py_Py3kWarning. Then do the > extra logic and execution speed. In 3.0, it's always a PendingDeprecationWarning, so that won't work. The test needs to be: if not recursing and warning_is_not_suppressed: warn() The recursion test is expensive if using thread local storage; the warning suppressed test looks expensive, too. So there's no quick short circuit test that I see. Of course all of this is just hot air until coded and benchmarked. I'll cook up a patch, but it will probably not be ready before the next alpha releases. |
|
|
| msg66350 - (view) |
Author: Neal Norwitz (nnorwitz) * |
Date: 2008-05-07 04:03 |
| Why not use the normal recursion check mechanism? Specifically, if (Py_EnterRecursiveCall("unicode % ")) return NULL; // err = Warn(); Py_LeaveRecursiveCall(); I don't see where the problem with threads comes in. The GIL is held and shouldn't be released during this call. That may not be quite true (it's conceivable the GIL is released when warning). I'm not sure what happens with the I/O system at this point, it's possible that releases the GIL. However, if GIL is released and re-acquired in PyWarn_WarnEx() there are probably bigger issues than this patch that will need to be addressed. Note that since the warnings module is now implemented in C, this should be easier to deal with. Using the macros above in essence uses TLS, but through Python's PyThreadState. |
|
|
| msg66361 - (view) |
Author: Eric V. Smith (eric.smith) * |
Date: 2008-05-07 15:33 |
| I'm not sure Py_EnterRecursiveCall is what I want, because that allows the recursion to happen, but just aborts it when it gets too deep. What I want to achieve is to have the warning not called if it's the warning that's being formatted. I coded this up and couldn't get it to do the right thing. I think a better approach will be to remove % formatting from warnings.py. I think that will remove the need for any checks at all. I'll investigate that. |
|
|
| msg66365 - (view) |
Author: Benjamin Peterson (benjamin.peterson) * |
Date: 2008-05-07 18:49 |
| On Wed, May 7, 2008 at 10:33 AM, Eric Smith <report@bugs.python.org> wrote: > I think a better approach will be to remove % formatting from > warnings.py. I think that will remove the need for any checks at all. > I'll investigate that. That would make user code warning that uses '%"' brittle. However, if we warn about it, I think it's ok. |
|
|
| msg66384 - (view) |
Author: Eric V. Smith (eric.smith) * |
Date: 2008-05-08 01:26 |
| > That would make user code warning that uses '%"' brittle. However, if > we warn about it, I think it's ok. True enough. Then I think we should go with: 1. Use .format() in the warnings module. 2. Tell the users not to use % formatting in user code for warnings. 3. Add my original, simple, global check for recursion. It will work incorrectly in an insignificant number of cases, and will typically result in at least one warning about % formatting, anyway. I'll have a patch ready soon. |
|
|
| msg66466 - (view) |
Author: Eric V. Smith (eric.smith) * |
Date: 2008-05-09 12:57 |
| The attached patch (percent_formatting_pending_deprecation_0.diff) adds both the simple global lock to unicode_mod() and changes warnings.py to use .format() instead of % formatting. I think this is good enough. We should add a warning to the docs saying user code used for warnings should also avoid % formatting. The only problem they'll see, however, is dropping an occasional warning in a multi-threaded app. But I'm not sure yet exactly where this mention would go. I also don't have any tests for this. I haven't found a test of a PendingDeprecationWarning. The biggest problem is that this patch breaks test_doctest with "RuntimeError: dictionary changed size during iteration". I've tried to find out why, but so far it escapes me. If someone else could look at the issue, I'd appreciate it. |
|
|
| msg70003 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2008-07-19 03:59 |
| I think this is premature until be know for sure that % formatting will in-fact be deprecated in Py3.1. Time will tell how well the new format options get accepted. Likewise, we'll learn more about how readily legacy code can get converted. Guido, can you pronouce? |
|
|
| msg70007 - (view) |
Author: Eric V. Smith (eric.smith) * |
Date: 2008-07-19 08:21 |
| I agree. That's one of the reasons I un-assigned it to me. Well, that and I couldn't get it to pass all tests. |
|
|
| msg79230 - (view) |
Author: Guido van Rossum (gvanrossum) * |
Date: 2009-01-06 05:04 |
| Not it. |
|
|
| msg79243 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2009-01-06 10:00 |
| Am closing this one. It is pointless and counter-productive unless there is a firm decision that % formatting is definitely going away (and migration tools have been developed). Right now, it looks like there is some chance that it might pull through for a few versions. |
|
|