classification
Title: asyncio: remove _TracebackLogger
Type: Stage: resolved
Components: Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: vstinner Nosy List: gvanrossum, pitrou, python-dev, vstinner
Priority: normal Keywords: patch

Created on 2013-12-12 23:02 by vstinner, last changed 2013-12-20 23:21 by vstinner. This issue is now closed.

Files
File name Uploaded Description Edit
asyncio_log_traceback.patch vstinner, 2013-12-12 23:02 review
asyncio_log_traceback-2.patch vstinner, 2013-12-13 00:15 review
asyncio_log_traceback-3.patch vstinner, 2013-12-19 15:52 review
asyncio_log_traceback-4.patch vstinner, 2013-12-19 19:56 review
asyncio_log_traceback-5.patch vstinner, 2013-12-19 20:22 review
asyncio_defer_format_tb.patch vstinner, 2013-12-20 09:55 review
Messages (14)
msg205988 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2013-12-12 23:02
Thanks to the PEP 442 (Safe object finalization), _TracebackLogger helper of asyncio.Futures becomes useless.

Attached patch removes it.

--

If you agree to diverge code with Tulip project, the Python 3.3 code of WriteTransport.writelines can also be removed:

        if not PY34:
            # In Python 3.3, bytes.join() doesn't handle memoryview.
            list_of_data = (
                bytes(data) if isinstance(data, memoryview) else data
                for data in list_of_data)
msg205990 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2013-12-12 23:46
I'm sorry, I really don't want the asyncio code for 3.3 and 3.4 to diverge just yet. It would make keeping the two versions in sync just so much harder.

I'm happy with something that checks for the version and either adds the __del__ method (for 3.4) or uses the _TracebackLogger (for 3.3).
msg205992 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2013-12-13 00:15
> I'm happy with something that checks for the version and either adds the __del__ method (for 3.4) or uses the _TracebackLogger (for 3.3).

Ok. Here is a patch. The class is still defined on Python 3.4. You may move the definition of the class in a "if not PY34:" class. I did not the change to have a shorter patch.
msg206625 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2013-12-19 15:52
Updated patch to address Guido's comments.
msg206627 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2013-12-19 17:47
So the new patch is fine, but I still think it's confusing that the _tb_logger variable has a different type depending on the Python version.  If you really don't want to fix this, just go ahead and check in, it's not a blocker.
msg206640 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2013-12-19 19:56
"So the new patch is fine, but I still think it's confusing that the _tb_logger variable has a different type depending on the Python version."

To be honest, I'm also concerned by this strange variable :-) Here is a new fix which reuses the name used in the first patch (_traceback).
msg206644 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2013-12-19 20:22
asyncio_log_traceback-5.patch: new try :-)
msg206648 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2013-12-19 20:36
Looks good. I can fix that long line myself. :-)
msg206652 - (view) Author: Roundup Robot (python-dev) Date: 2013-12-19 21:43
New changeset 5e9728ebb1d3 by Victor Stinner in branch 'default':
Close #19967: Thanks to the PEP 442, asyncio.Future can use a destructor in
http://hg.python.org/cpython/rev/5e9728ebb1d3
msg206656 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-12-20 01:09
I think this patch is bad and should be reverted. It always calls traceback.format_exception() which is an expensive operation, while the _TracebackLogger takes care to call it only when necessary.
msg206657 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2013-12-20 01:19
Eew. You're right. Sorry I didn't see this.
msg206673 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2013-12-20 09:55
> I think this patch is bad and should be reverted. It always calls traceback.format_exception() which is an expensive operation, while the _TracebackLogger takes care to call it only when necessary.

Oh, I didn't notice that, and I agree that the new code is inefficient.

Since the Future object does not release the reference to the exception after result() or exception() has been called, there is no need to preformat the exception. It can be done in the destructor.

Attached asyncio_defer_format_tb.patch implements that.

Future.set_exception() creates a reference cycle. I created the issue #20032 to discuss that.
msg206714 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2013-12-20 22:41
Victor, can you commit the fix (with my suggested improvement)?
msg206716 - (view) Author: Roundup Robot (python-dev) Date: 2013-12-20 23:20
New changeset 06ed1691efdc by Victor Stinner in branch 'default':
Issue #19967: Defer the formating of the traceback in asyncio.Future destructor
http://hg.python.org/cpython/rev/06ed1691efdc
History
Date User Action Args
2013-12-20 23:21:31vstinnersetstatus: open -> closed
resolution: fixed
2013-12-20 23:20:41python-devsetmessages: + msg206716
2013-12-20 22:41:08gvanrossumsetmessages: + msg206714
2013-12-20 09:55:28vstinnersetfiles: + asyncio_defer_format_tb.patch
resolution: fixed -> (no value)
messages: + msg206673
2013-12-20 01:19:37gvanrossumsetmessages: + msg206657
2013-12-20 01:09:26pitrousetstatus: closed -> open
assignee: vstinner
messages: + msg206656
2013-12-19 21:43:21python-devsetstatus: open -> closed

nosy: + python-dev
messages: + msg206652

resolution: fixed
stage: resolved
2013-12-19 20:36:10gvanrossumsetmessages: + msg206648
2013-12-19 20:22:09vstinnersetfiles: + asyncio_log_traceback-5.patch

messages: + msg206644
2013-12-19 19:56:22vstinnersetfiles: + asyncio_log_traceback-4.patch

messages: + msg206640
2013-12-19 17:47:51gvanrossumsetmessages: + msg206627
2013-12-19 15:52:33vstinnersetfiles: + asyncio_log_traceback-3.patch

messages: + msg206625
2013-12-13 00:15:20vstinnersetfiles: + asyncio_log_traceback-2.patch

messages: + msg205992
2013-12-12 23:46:41gvanrossumsetmessages: + msg205990
2013-12-12 23:02:49vstinnercreate