Mailman 3 September 2022 - Python-checkins

GH-97592: Fix crash in C remove_done_callback due to evil code (GH-97660)
by miss-islington 30 Sep '22

30 Sep '22

https://github.com/python/cpython/commit/54bbb5e3363ef3634f1fd7521ba3f3c42c… commit: 54bbb5e3363ef3634f1fd7521ba3f3c42c81a331 branch: 3.10 author: Miss Islington (bot) <31488909+miss-islington(a)users.noreply.github.com> committer: miss-islington <31488909+miss-islington(a)users.noreply.github.com> date: 2022-09-30T13:28:27-07:00 summary: GH-97592: Fix crash in C remove_done_callback due to evil code (GH-97660) Evil code could cause fut_callbacks to be cleared when PyObject_RichCompareBool is called. (cherry picked from commit 63780f4599acc2c5ee8af5f37ab76c162ad21065) Co-authored-by: Guido van Rossum <guido(a)python.org> files: A Misc/NEWS.d/next/Library/2022-09-29-23-22-24.gh-issue-97592.tpJg_J.rst M Lib/test/test_asyncio/test_futures.py M Modules/_asynciomodule.c diff --git a/Lib/test/test_asyncio/test_futures.py b/Lib/test/test_asyncio/test_futures.py index 838147b1e65f..012674260370 100644 --- a/Lib/test/test_asyncio/test_futures.py +++ b/Lib/test/test_asyncio/test_futures.py @@ -827,6 +827,21 @@ def __eq__(self, other): fut.remove_done_callback(evil()) + def test_remove_done_callbacks_list_clear(self): + # see https://github.com/python/cpython/issues/97592 for details + + fut = self._new_future() + fut.add_done_callback(str) + + for _ in range(63): + fut.add_done_callback(id) + + class evil: + def __eq__(self, other): + fut.remove_done_callback(other) + + fut.remove_done_callback(evil()) + def test_schedule_callbacks_list_mutation_1(self): # see http://bugs.python.org/issue28963 for details diff --git a/Misc/NEWS.d/next/Library/2022-09-29-23-22-24.gh-issue-97592.tpJg_J.rst b/Misc/NEWS.d/next/Library/2022-09-29-23-22-24.gh-issue-97592.tpJg_J.rst new file mode 100644 index 000000000000..aa245cf94400 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2022-09-29-23-22-24.gh-issue-97592.tpJg_J.rst @@ -0,0 +1 @@ +Avoid a crash in the C version of :meth:`asyncio.Future.remove_done_callback` when an evil argument is passed. diff --git a/Modules/_asynciomodule.c b/Modules/_asynciomodule.c index 1f6de2177a37..c627382a53f0 100644 --- a/Modules/_asynciomodule.c +++ b/Modules/_asynciomodule.c @@ -1042,7 +1042,11 @@ _asyncio_Future_remove_done_callback(FutureObj *self, PyObject *fn) return NULL; } - for (i = 0; i < PyList_GET_SIZE(self->fut_callbacks); i++) { + // Beware: PyObject_RichCompareBool below may change fut_callbacks. + // See GH-97592. + for (i = 0; + self->fut_callbacks != NULL && i < PyList_GET_SIZE(self->fut_callbacks); + i++) { int ret; PyObject *item = PyList_GET_ITEM(self->fut_callbacks, i); Py_INCREF(item); @@ -1061,7 +1065,8 @@ _asyncio_Future_remove_done_callback(FutureObj *self, PyObject *fn) } } - if (j == 0) { + // Note: fut_callbacks may have been cleared. + if (j == 0 || self->fut_callbacks == NULL) { Py_CLEAR(self->fut_callbacks); Py_DECREF(newlist); return PyLong_FromSsize_t(len + cleared_callback0);

1 0

GH-96827: Don't touch closed loops from executor threads (GH-96837)
by miss-islington 30 Sep '22

30 Sep '22

https://github.com/python/cpython/commit/a5c503f296a104d8b5b2e917a2f1ef71be… commit: a5c503f296a104d8b5b2e917a2f1ef71be888e1b branch: 3.10 author: Miss Islington (bot) <31488909+miss-islington(a)users.noreply.github.com> committer: miss-islington <31488909+miss-islington(a)users.noreply.github.com> date: 2022-09-30T13:25:06-07:00 summary: GH-96827: Don't touch closed loops from executor threads (GH-96837) * When chaining futures, skip callback if loop closed. * When shutting down an executor, don't wake a closed loop. (cherry picked from commit e9d63760fea8748638f6e495b5b07bd1805c9591) Co-authored-by: Guido van Rossum <guido(a)python.org> files: A Misc/NEWS.d/next/Library/2022-09-30-15-56-20.gh-issue-96827.lzy1iw.rst M Lib/asyncio/base_events.py M Lib/asyncio/futures.py diff --git a/Lib/asyncio/base_events.py b/Lib/asyncio/base_events.py index ea10399b9413..1cc26ee7ccf0 100644 --- a/Lib/asyncio/base_events.py +++ b/Lib/asyncio/base_events.py @@ -573,9 +573,11 @@ async def shutdown_default_executor(self): def _do_shutdown(self, future): try: self._default_executor.shutdown(wait=True) - self.call_soon_threadsafe(future.set_result, None) + if not self.is_closed(): + self.call_soon_threadsafe(future.set_result, None) except Exception as ex: - self.call_soon_threadsafe(future.set_exception, ex) + if not self.is_closed(): + self.call_soon_threadsafe(future.set_exception, ex) def _check_running(self): if self.is_running(): diff --git a/Lib/asyncio/futures.py b/Lib/asyncio/futures.py index 48a32f868385..5d00ab4c8d31 100644 --- a/Lib/asyncio/futures.py +++ b/Lib/asyncio/futures.py @@ -396,6 +396,8 @@ def _call_set_state(source): if dest_loop is None or dest_loop is source_loop: _set_state(destination, source) else: + if dest_loop.is_closed(): + return dest_loop.call_soon_threadsafe(_set_state, destination, source) destination.add_done_callback(_call_check_cancel) diff --git a/Misc/NEWS.d/next/Library/2022-09-30-15-56-20.gh-issue-96827.lzy1iw.rst b/Misc/NEWS.d/next/Library/2022-09-30-15-56-20.gh-issue-96827.lzy1iw.rst new file mode 100644 index 000000000000..159ab32ffbfc --- /dev/null +++ b/Misc/NEWS.d/next/Library/2022-09-30-15-56-20.gh-issue-96827.lzy1iw.rst @@ -0,0 +1 @@ +Avoid spurious tracebacks from :mod:`asyncio` when default executor cleanup is delayed until after the event loop is closed (e.g. as the result of a keyboard interrupt).

1 0

GH-96827: Don't touch closed loops from executor threads (GH-96837)
by miss-islington 30 Sep '22

30 Sep '22

https://github.com/python/cpython/commit/95609525de4efd4125390ab8ae82af7bc9… commit: 95609525de4efd4125390ab8ae82af7bc908cc0d branch: 3.11 author: Miss Islington (bot) <31488909+miss-islington(a)users.noreply.github.com> committer: miss-islington <31488909+miss-islington(a)users.noreply.github.com> date: 2022-09-30T13:23:33-07:00 summary: GH-96827: Don't touch closed loops from executor threads (GH-96837) * When chaining futures, skip callback if loop closed. * When shutting down an executor, don't wake a closed loop. (cherry picked from commit e9d63760fea8748638f6e495b5b07bd1805c9591) Co-authored-by: Guido van Rossum <guido(a)python.org> files: A Misc/NEWS.d/next/Library/2022-09-30-15-56-20.gh-issue-96827.lzy1iw.rst M Lib/asyncio/base_events.py M Lib/asyncio/futures.py diff --git a/Lib/asyncio/base_events.py b/Lib/asyncio/base_events.py index fa00bf9a2ca0..7c06e48a671a 100644 --- a/Lib/asyncio/base_events.py +++ b/Lib/asyncio/base_events.py @@ -577,9 +577,11 @@ async def shutdown_default_executor(self): def _do_shutdown(self, future): try: self._default_executor.shutdown(wait=True) - self.call_soon_threadsafe(future.set_result, None) + if not self.is_closed(): + self.call_soon_threadsafe(future.set_result, None) except Exception as ex: - self.call_soon_threadsafe(future.set_exception, ex) + if not self.is_closed(): + self.call_soon_threadsafe(future.set_exception, ex) def _check_running(self): if self.is_running(): diff --git a/Lib/asyncio/futures.py b/Lib/asyncio/futures.py index 4bd9629e9eb6..39776e3c2cce 100644 --- a/Lib/asyncio/futures.py +++ b/Lib/asyncio/futures.py @@ -404,6 +404,8 @@ def _call_set_state(source): if dest_loop is None or dest_loop is source_loop: _set_state(destination, source) else: + if dest_loop.is_closed(): + return dest_loop.call_soon_threadsafe(_set_state, destination, source) destination.add_done_callback(_call_check_cancel) diff --git a/Misc/NEWS.d/next/Library/2022-09-30-15-56-20.gh-issue-96827.lzy1iw.rst b/Misc/NEWS.d/next/Library/2022-09-30-15-56-20.gh-issue-96827.lzy1iw.rst new file mode 100644 index 000000000000..159ab32ffbfc --- /dev/null +++ b/Misc/NEWS.d/next/Library/2022-09-30-15-56-20.gh-issue-96827.lzy1iw.rst @@ -0,0 +1 @@ +Avoid spurious tracebacks from :mod:`asyncio` when default executor cleanup is delayed until after the event loop is closed (e.g. as the result of a keyboard interrupt).

1 0

GH-97592: Fix crash in C remove_done_callback due to evil code (GH-97660)
by miss-islington 30 Sep '22

30 Sep '22

https://github.com/python/cpython/commit/3124618fd5be9c00b1521c8a980f6aea02… commit: 3124618fd5be9c00b1521c8a980f6aea02dc1a48 branch: 3.11 author: Miss Islington (bot) <31488909+miss-islington(a)users.noreply.github.com> committer: miss-islington <31488909+miss-islington(a)users.noreply.github.com> date: 2022-09-30T13:22:48-07:00 summary: GH-97592: Fix crash in C remove_done_callback due to evil code (GH-97660) Evil code could cause fut_callbacks to be cleared when PyObject_RichCompareBool is called. (cherry picked from commit 63780f4599acc2c5ee8af5f37ab76c162ad21065) Co-authored-by: Guido van Rossum <guido(a)python.org> files: A Misc/NEWS.d/next/Library/2022-09-29-23-22-24.gh-issue-97592.tpJg_J.rst M Lib/test/test_asyncio/test_futures.py M Modules/_asynciomodule.c diff --git a/Lib/test/test_asyncio/test_futures.py b/Lib/test/test_asyncio/test_futures.py index f4a46ec90a16..c4c934f547b4 100644 --- a/Lib/test/test_asyncio/test_futures.py +++ b/Lib/test/test_asyncio/test_futures.py @@ -832,6 +832,21 @@ def __eq__(self, other): fut.remove_done_callback(evil()) + def test_remove_done_callbacks_list_clear(self): + # see https://github.com/python/cpython/issues/97592 for details + + fut = self._new_future() + fut.add_done_callback(str) + + for _ in range(63): + fut.add_done_callback(id) + + class evil: + def __eq__(self, other): + fut.remove_done_callback(other) + + fut.remove_done_callback(evil()) + def test_schedule_callbacks_list_mutation_1(self): # see http://bugs.python.org/issue28963 for details diff --git a/Misc/NEWS.d/next/Library/2022-09-29-23-22-24.gh-issue-97592.tpJg_J.rst b/Misc/NEWS.d/next/Library/2022-09-29-23-22-24.gh-issue-97592.tpJg_J.rst new file mode 100644 index 000000000000..aa245cf94400 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2022-09-29-23-22-24.gh-issue-97592.tpJg_J.rst @@ -0,0 +1 @@ +Avoid a crash in the C version of :meth:`asyncio.Future.remove_done_callback` when an evil argument is passed. diff --git a/Modules/_asynciomodule.c b/Modules/_asynciomodule.c index 10679be2ef19..49f1ce33dc72 100644 --- a/Modules/_asynciomodule.c +++ b/Modules/_asynciomodule.c @@ -1052,7 +1052,11 @@ _asyncio_Future_remove_done_callback(FutureObj *self, PyObject *fn) return NULL; } - for (i = 0; i < PyList_GET_SIZE(self->fut_callbacks); i++) { + // Beware: PyObject_RichCompareBool below may change fut_callbacks. + // See GH-97592. + for (i = 0; + self->fut_callbacks != NULL && i < PyList_GET_SIZE(self->fut_callbacks); + i++) { int ret; PyObject *item = PyList_GET_ITEM(self->fut_callbacks, i); Py_INCREF(item); @@ -1071,7 +1075,8 @@ _asyncio_Future_remove_done_callback(FutureObj *self, PyObject *fn) } } - if (j == 0) { + // Note: fut_callbacks may have been cleared. + if (j == 0 || self->fut_callbacks == NULL) { Py_CLEAR(self->fut_callbacks); Py_DECREF(newlist); return PyLong_FromSsize_t(len + cleared_callback0);

1 0

GH-97592: Fix crash in C remove_done_callback due to evil code (#97660)
by gvanrossum 30 Sep '22

30 Sep '22

https://github.com/python/cpython/commit/63780f4599acc2c5ee8af5f37ab76c162a… commit: 63780f4599acc2c5ee8af5f37ab76c162ad21065 branch: main author: Guido van Rossum <guido(a)python.org> committer: gvanrossum <gvanrossum(a)gmail.com> date: 2022-09-30T12:57:09-07:00 summary: GH-97592: Fix crash in C remove_done_callback due to evil code (#97660) Evil code could cause fut_callbacks to be cleared when PyObject_RichCompareBool is called. files: A Misc/NEWS.d/next/Library/2022-09-29-23-22-24.gh-issue-97592.tpJg_J.rst M Lib/test/test_asyncio/test_futures.py M Modules/_asynciomodule.c diff --git a/Lib/test/test_asyncio/test_futures.py b/Lib/test/test_asyncio/test_futures.py index 11d427393080..3dc6b658cfae 100644 --- a/Lib/test/test_asyncio/test_futures.py +++ b/Lib/test/test_asyncio/test_futures.py @@ -837,6 +837,21 @@ def __eq__(self, other): fut.remove_done_callback(evil()) + def test_remove_done_callbacks_list_clear(self): + # see https://github.com/python/cpython/issues/97592 for details + + fut = self._new_future() + fut.add_done_callback(str) + + for _ in range(63): + fut.add_done_callback(id) + + class evil: + def __eq__(self, other): + fut.remove_done_callback(other) + + fut.remove_done_callback(evil()) + def test_schedule_callbacks_list_mutation_1(self): # see http://bugs.python.org/issue28963 for details diff --git a/Misc/NEWS.d/next/Library/2022-09-29-23-22-24.gh-issue-97592.tpJg_J.rst b/Misc/NEWS.d/next/Library/2022-09-29-23-22-24.gh-issue-97592.tpJg_J.rst new file mode 100644 index 000000000000..aa245cf94400 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2022-09-29-23-22-24.gh-issue-97592.tpJg_J.rst @@ -0,0 +1 @@ +Avoid a crash in the C version of :meth:`asyncio.Future.remove_done_callback` when an evil argument is passed. diff --git a/Modules/_asynciomodule.c b/Modules/_asynciomodule.c index 5a5881b873e2..909171150bdd 100644 --- a/Modules/_asynciomodule.c +++ b/Modules/_asynciomodule.c @@ -1052,7 +1052,11 @@ _asyncio_Future_remove_done_callback(FutureObj *self, PyObject *fn) return NULL; } - for (i = 0; i < PyList_GET_SIZE(self->fut_callbacks); i++) { + // Beware: PyObject_RichCompareBool below may change fut_callbacks. + // See GH-97592. + for (i = 0; + self->fut_callbacks != NULL && i < PyList_GET_SIZE(self->fut_callbacks); + i++) { int ret; PyObject *item = PyList_GET_ITEM(self->fut_callbacks, i); Py_INCREF(item); @@ -1071,7 +1075,8 @@ _asyncio_Future_remove_done_callback(FutureObj *self, PyObject *fn) } } - if (j == 0) { + // Note: fut_callbacks may have been cleared. + if (j == 0 || self->fut_callbacks == NULL) { Py_CLEAR(self->fut_callbacks); Py_DECREF(newlist); return PyLong_FromSsize_t(len + cleared_callback0);

1 0

GH-96827: Don't touch closed loops from executor threads (#96837)
by gvanrossum 30 Sep '22

30 Sep '22

https://github.com/python/cpython/commit/e9d63760fea8748638f6e495b5b07bd180… commit: e9d63760fea8748638f6e495b5b07bd1805c9591 branch: main author: Guido van Rossum <guido(a)python.org> committer: gvanrossum <gvanrossum(a)gmail.com> date: 2022-09-30T12:55:40-07:00 summary: GH-96827: Don't touch closed loops from executor threads (#96837) * When chaining futures, skip callback if loop closed. * When shutting down an executor, don't wake a closed loop. files: A Misc/NEWS.d/next/Library/2022-09-30-15-56-20.gh-issue-96827.lzy1iw.rst M Lib/asyncio/base_events.py M Lib/asyncio/futures.py diff --git a/Lib/asyncio/base_events.py b/Lib/asyncio/base_events.py index 9c9d98dbb9c5..2df9dcac8f72 100644 --- a/Lib/asyncio/base_events.py +++ b/Lib/asyncio/base_events.py @@ -588,9 +588,11 @@ async def shutdown_default_executor(self, timeout=None): def _do_shutdown(self, future): try: self._default_executor.shutdown(wait=True) - self.call_soon_threadsafe(future.set_result, None) + if not self.is_closed(): + self.call_soon_threadsafe(future.set_result, None) except Exception as ex: - self.call_soon_threadsafe(future.set_exception, ex) + if not self.is_closed(): + self.call_soon_threadsafe(future.set_exception, ex) def _check_running(self): if self.is_running(): diff --git a/Lib/asyncio/futures.py b/Lib/asyncio/futures.py index 4bd9629e9eb6..39776e3c2cce 100644 --- a/Lib/asyncio/futures.py +++ b/Lib/asyncio/futures.py @@ -404,6 +404,8 @@ def _call_set_state(source): if dest_loop is None or dest_loop is source_loop: _set_state(destination, source) else: + if dest_loop.is_closed(): + return dest_loop.call_soon_threadsafe(_set_state, destination, source) destination.add_done_callback(_call_check_cancel) diff --git a/Misc/NEWS.d/next/Library/2022-09-30-15-56-20.gh-issue-96827.lzy1iw.rst b/Misc/NEWS.d/next/Library/2022-09-30-15-56-20.gh-issue-96827.lzy1iw.rst new file mode 100644 index 000000000000..159ab32ffbfc --- /dev/null +++ b/Misc/NEWS.d/next/Library/2022-09-30-15-56-20.gh-issue-96827.lzy1iw.rst @@ -0,0 +1 @@ +Avoid spurious tracebacks from :mod:`asyncio` when default executor cleanup is delayed until after the event loop is closed (e.g. as the result of a keyboard interrupt).

1 0

gh-87597: Document TimeoutExpired.stdout & .stderr types (GH-97685)
by miss-islington 30 Sep '22

30 Sep '22

https://github.com/python/cpython/commit/cdc01eaed9d19c17b4f01c3da3ca125c49… commit: cdc01eaed9d19c17b4f01c3da3ca125c49fdffa5 branch: 3.11 author: Miss Islington (bot) <31488909+miss-islington(a)users.noreply.github.com> committer: miss-islington <31488909+miss-islington(a)users.noreply.github.com> date: 2022-09-30T10:54:12-07:00 summary: gh-87597: Document TimeoutExpired.stdout & .stderr types (GH-97685) This documents the behavior that has always been the case since timeout support was introduced in Python 3.3. (cherry picked from commit b05dd796492160c37c9e15e3882f699f411b3461) Co-authored-by: Gregory P. Smith <greg(a)krypto.org> files: M Doc/library/subprocess.rst diff --git a/Doc/library/subprocess.rst b/Doc/library/subprocess.rst index 43d6ffceee8f..dee5bd879db5 100644 --- a/Doc/library/subprocess.rst +++ b/Doc/library/subprocess.rst @@ -193,7 +193,10 @@ underlying :class:`Popen` interface can be used directly. .. attribute:: output Output of the child process if it was captured by :func:`run` or - :func:`check_output`. Otherwise, ``None``. + :func:`check_output`. Otherwise, ``None``. This is always + :class:`bytes` when any output was captured regardless of the + ``text=True`` setting. It may remain ``None`` instead of ``b''`` + when no output was observed. .. attribute:: stdout @@ -202,7 +205,9 @@ underlying :class:`Popen` interface can be used directly. .. attribute:: stderr Stderr output of the child process if it was captured by :func:`run`. - Otherwise, ``None``. + Otherwise, ``None``. This is always :class:`bytes` when stderr output + was captured regardless of the ``text=True`` setting. It may remain + ``None`` instead of ``b''`` when no stderr output was observed. .. versionadded:: 3.3

1 0

gh-87597: Document TimeoutExpired.stdout & .stderr types (GH-97685)
by miss-islington 30 Sep '22

30 Sep '22

https://github.com/python/cpython/commit/6d7dbcc0b365c9f227a4c299211c068b3d… commit: 6d7dbcc0b365c9f227a4c299211c068b3da54a90 branch: 3.10 author: Miss Islington (bot) <31488909+miss-islington(a)users.noreply.github.com> committer: miss-islington <31488909+miss-islington(a)users.noreply.github.com> date: 2022-09-30T10:53:07-07:00 summary: gh-87597: Document TimeoutExpired.stdout & .stderr types (GH-97685) This documents the behavior that has always been the case since timeout support was introduced in Python 3.3. (cherry picked from commit b05dd796492160c37c9e15e3882f699f411b3461) Co-authored-by: Gregory P. Smith <greg(a)krypto.org> files: M Doc/library/subprocess.rst diff --git a/Doc/library/subprocess.rst b/Doc/library/subprocess.rst index 45fcfc79522c..f35a8204ce03 100644 --- a/Doc/library/subprocess.rst +++ b/Doc/library/subprocess.rst @@ -192,7 +192,10 @@ underlying :class:`Popen` interface can be used directly. .. attribute:: output Output of the child process if it was captured by :func:`run` or - :func:`check_output`. Otherwise, ``None``. + :func:`check_output`. Otherwise, ``None``. This is always + :class:`bytes` when any output was captured regardless of the + ``text=True`` setting. It may remain ``None`` instead of ``b''`` + when no output was observed. .. attribute:: stdout @@ -201,7 +204,9 @@ underlying :class:`Popen` interface can be used directly. .. attribute:: stderr Stderr output of the child process if it was captured by :func:`run`. - Otherwise, ``None``. + Otherwise, ``None``. This is always :class:`bytes` when stderr output + was captured regardless of the ``text=True`` setting. It may remain + ``None`` instead of ``b''`` when no stderr output was observed. .. versionadded:: 3.3

1 0

gh-87597: Document TimeoutExpired.stdout & .stderr types (#97685)
by gpshead 30 Sep '22

30 Sep '22

https://github.com/python/cpython/commit/b05dd796492160c37c9e15e3882f699f41… commit: b05dd796492160c37c9e15e3882f699f411b3461 branch: main author: Gregory P. Smith <greg(a)krypto.org> committer: gpshead <greg(a)krypto.org> date: 2022-09-30T10:45:47-07:00 summary: gh-87597: Document TimeoutExpired.stdout & .stderr types (#97685) This documents the behavior that has always been the case since timeout support was introduced in Python 3.3. files: M Doc/library/subprocess.rst diff --git a/Doc/library/subprocess.rst b/Doc/library/subprocess.rst index 43d6ffceee8f..dee5bd879db5 100644 --- a/Doc/library/subprocess.rst +++ b/Doc/library/subprocess.rst @@ -193,7 +193,10 @@ underlying :class:`Popen` interface can be used directly. .. attribute:: output Output of the child process if it was captured by :func:`run` or - :func:`check_output`. Otherwise, ``None``. + :func:`check_output`. Otherwise, ``None``. This is always + :class:`bytes` when any output was captured regardless of the + ``text=True`` setting. It may remain ``None`` instead of ``b''`` + when no output was observed. .. attribute:: stdout @@ -202,7 +205,9 @@ underlying :class:`Popen` interface can be used directly. .. attribute:: stderr Stderr output of the child process if it was captured by :func:`run`. - Otherwise, ``None``. + Otherwise, ``None``. This is always :class:`bytes` when stderr output + was captured regardless of the ``text=True`` setting. It may remain + ``None`` instead of ``b''`` when no stderr output was observed. .. versionadded:: 3.3

1 0

bpo-35675: IDLE - separate config_key window and frame (GH-11427)
by miss-islington 30 Sep '22

30 Sep '22

https://github.com/python/cpython/commit/b61a51a450f8115ec1f174a00f4e702371… commit: b61a51a450f8115ec1f174a00f4e70237148e92b branch: 3.10 author: Miss Islington (bot) <31488909+miss-islington(a)users.noreply.github.com> committer: miss-islington <31488909+miss-islington(a)users.noreply.github.com> date: 2022-09-30T10:27:14-07:00 summary: bpo-35675: IDLE - separate config_key window and frame (GH-11427) bpo-35598: IDLE: Refactor window and frame class Co-authored-by: Terry Jan Reedy <tjreedy(a)udel.edu> (cherry picked from commit 1cc308d03c1b44a0885a3c5f07d0786b49ea711d) Co-authored-by: Cheryl Sabella <cheryl.sabella(a)gmail.com> files: M Lib/idlelib/config_key.py M Lib/idlelib/configdialog.py M Lib/idlelib/idle_test/test_config_key.py M Lib/idlelib/idle_test/test_configdialog.py diff --git a/Lib/idlelib/config_key.py b/Lib/idlelib/config_key.py index 9ca3a156f4b9..bb07231cd590 100644 --- a/Lib/idlelib/config_key.py +++ b/Lib/idlelib/config_key.py @@ -41,32 +41,22 @@ def translate_key(key, modifiers): return f'Key-{key}' -class GetKeysDialog(Toplevel): +class GetKeysFrame(Frame): # Dialog title for invalid key sequence keyerror_title = 'Key Sequence Error' - def __init__(self, parent, title, action, current_key_sequences, - *, _htest=False, _utest=False): + def __init__(self, parent, action, current_key_sequences): """ parent - parent of this dialog - title - string which is the title of the popup dialog - action - string, the name of the virtual event these keys will be + action - the name of the virtual event these keys will be mapped to - current_key_sequences - list, a list of all key sequence lists + current_key_sequences - a list of all key sequence lists currently mapped to virtual events, for overlap checking - _htest - bool, change box location when running htest - _utest - bool, do not wait when running unittest """ - Toplevel.__init__(self, parent) - self.withdraw() # Hide while setting geometry. - self.configure(borderwidth=5) - self.resizable(height=False, width=False) - self.title(title) - self.transient(parent) - _setup_dialog(self) - self.grab_set() - self.protocol("WM_DELETE_WINDOW", self.cancel) + super().__init__(parent) + self['borderwidth'] = 2 + self['relief'] = 'sunken' self.parent = parent self.action = action self.current_key_sequences = current_key_sequences @@ -82,39 +72,14 @@ def __init__(self, parent, title, action, current_key_sequences, self.modifier_vars.append(variable) self.advanced = False self.create_widgets() - self.update_idletasks() - self.geometry( - "+%d+%d" % ( - parent.winfo_rootx() + - (parent.winfo_width()/2 - self.winfo_reqwidth()/2), - parent.winfo_rooty() + - ((parent.winfo_height()/2 - self.winfo_reqheight()/2) - if not _htest else 150) - ) ) # Center dialog over parent (or below htest box). - if not _utest: - self.deiconify() # Geometry set, unhide. - self.wait_window() def showerror(self, *args, **kwargs): # Make testing easier. Replace in #30751. messagebox.showerror(*args, **kwargs) def create_widgets(self): - self.frame = frame = Frame(self, borderwidth=2, relief='sunken') - frame.pack(side='top', expand=True, fill='both') - - frame_buttons = Frame(self) - frame_buttons.pack(side='bottom', fill='x') - - self.button_ok = Button(frame_buttons, text='OK', - width=8, command=self.ok) - self.button_ok.grid(row=0, column=0, padx=5, pady=5) - self.button_cancel = Button(frame_buttons, text='Cancel', - width=8, command=self.cancel) - self.button_cancel.grid(row=0, column=1, padx=5, pady=5) - # Basic entry key sequence. - self.frame_keyseq_basic = Frame(frame, name='keyseq_basic') + self.frame_keyseq_basic = Frame(self, name='keyseq_basic') self.frame_keyseq_basic.grid(row=0, column=0, sticky='nsew', padx=5, pady=5) basic_title = Label(self.frame_keyseq_basic, @@ -127,7 +92,7 @@ def create_widgets(self): basic_keys.pack(ipadx=5, ipady=5, fill='x') # Basic entry controls. - self.frame_controls_basic = Frame(frame) + self.frame_controls_basic = Frame(self) self.frame_controls_basic.grid(row=1, column=0, sticky='nsew', padx=5) # Basic entry modifiers. @@ -169,7 +134,7 @@ def create_widgets(self): self.button_clear.grid(row=2, column=0, columnspan=4) # Advanced entry key sequence. - self.frame_keyseq_advanced = Frame(frame, name='keyseq_advanced') + self.frame_keyseq_advanced = Frame(self, name='keyseq_advanced') self.frame_keyseq_advanced.grid(row=0, column=0, sticky='nsew', padx=5, pady=5) advanced_title = Label(self.frame_keyseq_advanced, justify='left', @@ -181,7 +146,7 @@ def create_widgets(self): self.advanced_keys.pack(fill='x') # Advanced entry help text. - self.frame_help_advanced = Frame(frame) + self.frame_help_advanced = Frame(self) self.frame_help_advanced.grid(row=1, column=0, sticky='nsew', padx=5) help_advanced = Label(self.frame_help_advanced, justify='left', text="Key bindings are specified using Tkinter keysyms as\n"+ @@ -196,7 +161,7 @@ def create_widgets(self): help_advanced.grid(row=0, column=0, sticky='nsew') # Switch between basic and advanced. - self.button_level = Button(frame, command=self.toggle_level, + self.button_level = Button(self, command=self.toggle_level, text='<< Basic Key Binding Entry') self.button_level.grid(row=2, column=0, stick='ew', padx=5, pady=5) self.toggle_level() @@ -257,7 +222,8 @@ def clear_key_seq(self): variable.set('') self.key_string.set('') - def ok(self, event=None): + def ok(self): + self.result = '' keys = self.key_string.get().strip() if not keys: self.showerror(title=self.keyerror_title, parent=self, @@ -265,13 +231,7 @@ def ok(self, event=None): return if (self.advanced or self.keys_ok(keys)) and self.bind_ok(keys): self.result = keys - self.grab_release() - self.destroy() - - def cancel(self, event=None): - self.result = '' - self.grab_release() - self.destroy() + return def keys_ok(self, keys): """Validity check on user's 'basic' keybinding selection. @@ -319,6 +279,73 @@ def bind_ok(self, keys): return True +class GetKeysWindow(Toplevel): + + def __init__(self, parent, title, action, current_key_sequences, + *, _htest=False, _utest=False): + """ + parent - parent of this dialog + title - string which is the title of the popup dialog + action - string, the name of the virtual event these keys will be + mapped to + current_key_sequences - list, a list of all key sequence lists + currently mapped to virtual events, for overlap checking + _htest - bool, change box location when running htest + _utest - bool, do not wait when running unittest + """ + super().__init__(parent) + self.withdraw() # Hide while setting geometry. + self['borderwidth'] = 5 + self.resizable(height=False, width=False) + # Needed for winfo_reqwidth(). + self.update_idletasks() + # Center dialog over parent (or below htest box). + x = (parent.winfo_rootx() + + (parent.winfo_width()//2 - self.winfo_reqwidth()//2)) + y = (parent.winfo_rooty() + + ((parent.winfo_height()//2 - self.winfo_reqheight()//2) + if not _htest else 150)) + self.geometry(f"+{x}+{y}") + + self.title(title) + self.frame = frame = GetKeysFrame(self, action, current_key_sequences) + self.protocol("WM_DELETE_WINDOW", self.cancel) + frame_buttons = Frame(self) + self.button_ok = Button(frame_buttons, text='OK', + width=8, command=self.ok) + self.button_cancel = Button(frame_buttons, text='Cancel', + width=8, command=self.cancel) + self.button_ok.grid(row=0, column=0, padx=5, pady=5) + self.button_cancel.grid(row=0, column=1, padx=5, pady=5) + frame.pack(side='top', expand=True, fill='both') + frame_buttons.pack(side='bottom', fill='x') + + self.transient(parent) + _setup_dialog(self) + self.grab_set() + if not _utest: + self.deiconify() # Geometry set, unhide. + self.wait_window() + + @property + def result(self): + return self.frame.result + + @result.setter + def result(self, value): + self.frame.result = value + + def ok(self, event=None): + self.frame.ok() + self.grab_release() + self.destroy() + + def cancel(self, event=None): + self.result = '' + self.grab_release() + self.destroy() + + if __name__ == '__main__': from unittest import main main('idlelib.idle_test.test_config_key', verbosity=2, exit=False) diff --git a/Lib/idlelib/configdialog.py b/Lib/idlelib/configdialog.py index 09f455c2500f..43349cb07f58 100644 --- a/Lib/idlelib/configdialog.py +++ b/Lib/idlelib/configdialog.py @@ -24,7 +24,7 @@ from tkinter import messagebox from idlelib.config import idleConf, ConfigChanges -from idlelib.config_key import GetKeysDialog +from idlelib.config_key import GetKeysWindow from idlelib.dynoption import DynOptionMenu from idlelib import macosx from idlelib.query import SectionName, HelpSource @@ -1397,7 +1397,7 @@ def get_new_keys(self): for event in key_set_changes: current_bindings[event] = key_set_changes[event].split() current_key_sequences = list(current_bindings.values()) - new_keys = GetKeysDialog(self, 'Get New Keys', bind_name, + new_keys = GetKeysWindow(self, 'Get New Keys', bind_name, current_key_sequences).result if new_keys: if self.keyset_source.get(): # Current key set is a built-in. diff --git a/Lib/idlelib/idle_test/test_config_key.py b/Lib/idlelib/idle_test/test_config_key.py index bf66cadf57cd..32f878b842b2 100644 --- a/Lib/idlelib/idle_test/test_config_key.py +++ b/Lib/idlelib/idle_test/test_config_key.py @@ -13,15 +13,13 @@ from idlelib.idle_test.mock_idle import Func from idlelib.idle_test.mock_tk import Mbox_func -gkd = config_key.GetKeysDialog - class ValidationTest(unittest.TestCase): "Test validation methods: ok, keys_ok, bind_ok." - class Validator(gkd): + class Validator(config_key.GetKeysFrame): def __init__(self, *args, **kwargs): - config_key.GetKeysDialog.__init__(self, *args, **kwargs) + super().__init__(*args, **kwargs) class list_keys_final: get = Func() self.list_keys_final = list_keys_final @@ -34,15 +32,14 @@ def setUpClass(cls): cls.root = Tk() cls.root.withdraw() keylist = [['<Key-F12>'], ['<Control-Key-x>', '<Control-Key-X>']] - cls.dialog = cls.Validator( - cls.root, 'Title', '<<Test>>', keylist, _utest=True) + cls.dialog = cls.Validator(cls.root, '<<Test>>', keylist) @classmethod def tearDownClass(cls): - cls.dialog.cancel() + del cls.dialog cls.root.update_idletasks() cls.root.destroy() - del cls.dialog, cls.root + del cls.root def setUp(self): self.dialog.showerror.message = '' @@ -111,14 +108,14 @@ def setUpClass(cls): requires('gui') cls.root = Tk() cls.root.withdraw() - cls.dialog = gkd(cls.root, 'Title', '<<Test>>', [], _utest=True) + cls.dialog = config_key.GetKeysFrame(cls.root, '<<Test>>', []) @classmethod def tearDownClass(cls): - cls.dialog.cancel() + del cls.dialog cls.root.update_idletasks() cls.root.destroy() - del cls.dialog, cls.root + del cls.root def test_toggle_level(self): dialog = self.dialog @@ -130,7 +127,7 @@ def stackorder(): this can be used to check whether a frame is above or below another one. """ - for index, child in enumerate(dialog.frame.winfo_children()): + for index, child in enumerate(dialog.winfo_children()): if child._name == 'keyseq_basic': basic = index if child._name == 'keyseq_advanced': @@ -161,7 +158,7 @@ def stackorder(): class KeySelectionTest(unittest.TestCase): "Test selecting key on Basic frames." - class Basic(gkd): + class Basic(config_key.GetKeysFrame): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) class list_keys_final: @@ -179,14 +176,14 @@ def setUpClass(cls): requires('gui') cls.root = Tk() cls.root.withdraw() - cls.dialog = cls.Basic(cls.root, 'Title', '<<Test>>', [], _utest=True) + cls.dialog = cls.Basic(cls.root, '<<Test>>', []) @classmethod def tearDownClass(cls): - cls.dialog.cancel() + del cls.dialog cls.root.update_idletasks() cls.root.destroy() - del cls.dialog, cls.root + del cls.root def setUp(self): self.dialog.clear_key_seq() @@ -206,7 +203,7 @@ def test_get_modifiers(self): dialog.modifier_checkbuttons['foo'].invoke() eq(gm(), ['BAZ']) - @mock.patch.object(gkd, 'get_modifiers') + @mock.patch.object(config_key.GetKeysFrame, 'get_modifiers') def test_build_key_string(self, mock_modifiers): dialog = self.dialog key = dialog.list_keys_final @@ -227,7 +224,7 @@ def test_build_key_string(self, mock_modifiers): dialog.build_key_string() eq(string(), '<mymod-test>') - @mock.patch.object(gkd, 'get_modifiers') + @mock.patch.object(config_key.GetKeysFrame, 'get_modifiers') def test_final_key_selected(self, mock_modifiers): dialog = self.dialog key = dialog.list_keys_final @@ -240,7 +237,7 @@ def test_final_key_selected(self, mock_modifiers): eq(string(), '<Shift-Key-braceleft>') -class CancelTest(unittest.TestCase): +class CancelWindowTest(unittest.TestCase): "Simulate user clicking [Cancel] button." @classmethod @@ -248,21 +245,89 @@ def setUpClass(cls): requires('gui') cls.root = Tk() cls.root.withdraw() - cls.dialog = gkd(cls.root, 'Title', '<<Test>>', [], _utest=True) + cls.dialog = config_key.GetKeysWindow( + cls.root, 'Title', '<<Test>>', [], _utest=True) @classmethod def tearDownClass(cls): cls.dialog.cancel() + del cls.dialog cls.root.update_idletasks() cls.root.destroy() - del cls.dialog, cls.root + del cls.root - def test_cancel(self): + @mock.patch.object(config_key.GetKeysFrame, 'ok') + def test_cancel(self, mock_frame_ok): self.assertEqual(self.dialog.winfo_class(), 'Toplevel') self.dialog.button_cancel.invoke() with self.assertRaises(TclError): self.dialog.winfo_class() self.assertEqual(self.dialog.result, '') + mock_frame_ok.assert_not_called() + + +class OKWindowTest(unittest.TestCase): + "Simulate user clicking [OK] button." + + @classmethod + def setUpClass(cls): + requires('gui') + cls.root = Tk() + cls.root.withdraw() + cls.dialog = config_key.GetKeysWindow( + cls.root, 'Title', '<<Test>>', [], _utest=True) + + @classmethod + def tearDownClass(cls): + cls.dialog.cancel() + del cls.dialog + cls.root.update_idletasks() + cls.root.destroy() + del cls.root + + @mock.patch.object(config_key.GetKeysFrame, 'ok') + def test_ok(self, mock_frame_ok): + self.assertEqual(self.dialog.winfo_class(), 'Toplevel') + self.dialog.button_ok.invoke() + with self.assertRaises(TclError): + self.dialog.winfo_class() + mock_frame_ok.assert_called() + + +class WindowResultTest(unittest.TestCase): + "Test window result get and set." + + @classmethod + def setUpClass(cls): + requires('gui') + cls.root = Tk() + cls.root.withdraw() + cls.dialog = config_key.GetKeysWindow( + cls.root, 'Title', '<<Test>>', [], _utest=True) + + @classmethod + def tearDownClass(cls): + cls.dialog.cancel() + del cls.dialog + cls.root.update_idletasks() + cls.root.destroy() + del cls.root + + def test_result(self): + dialog = self.dialog + eq = self.assertEqual + + dialog.result = '' + eq(dialog.result, '') + eq(dialog.frame.result,'') + + dialog.result = 'bar' + eq(dialog.result,'bar') + eq(dialog.frame.result,'bar') + + dialog.frame.result = 'foo' + eq(dialog.result, 'foo') + eq(dialog.frame.result,'foo') class HelperTest(unittest.TestCase): diff --git a/Lib/idlelib/idle_test/test_configdialog.py b/Lib/idlelib/idle_test/test_configdialog.py index 3005ce08c9bf..e5d5b4013fca 100644 --- a/Lib/idlelib/idle_test/test_configdialog.py +++ b/Lib/idlelib/idle_test/test_configdialog.py @@ -954,8 +954,8 @@ def test_set_keys_type(self): def test_get_new_keys(self): eq = self.assertEqual d = self.page - orig_getkeysdialog = configdialog.GetKeysDialog - gkd = configdialog.GetKeysDialog = Func(return_self=True) + orig_getkeysdialog = configdialog.GetKeysWindow + gkd = configdialog.GetKeysWindow = Func(return_self=True) gnkn = d.get_new_keys_name = Func() d.button_new_keys.state(('!disabled',)) @@ -997,7 +997,7 @@ def test_get_new_keys(self): eq(d.keybinding.get(), '<Key-p>') del d.get_new_keys_name - configdialog.GetKeysDialog = orig_getkeysdialog + configdialog.GetKeysWindow = orig_getkeysdialog def test_get_new_keys_name(self): orig_sectionname = configdialog.SectionName

1 0