From 6051f598008543780298e4c60fb3e331e53c66f5 Mon Sep 17 00:00:00 2001 From: Berker Peksag Date: Mon, 6 Aug 2018 21:54:16 +0300 Subject: [PATCH 1/7] bpo-2122: Make mmap.flush() behave same on all platforms --- Doc/library/mmap.rst | 8 ++++---- Lib/test/test_mmap.py | 16 +++++++++++----- .../2018-08-06-21-47-03.bpo-2122.GWdmrm.rst | 2 ++ Modules/mmapmodule.c | 6 +++++- 4 files changed, 22 insertions(+), 10 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2018-08-06-21-47-03.bpo-2122.GWdmrm.rst diff --git a/Doc/library/mmap.rst b/Doc/library/mmap.rst index ca09a6a3ca9955..f4bb46820996c2 100644 --- a/Doc/library/mmap.rst +++ b/Doc/library/mmap.rst @@ -191,12 +191,12 @@ To map anonymous memory, -1 should be passed as the fileno along with the length changes to the given range of bytes will be flushed to disk; otherwise, the whole extent of the mapping is flushed. - **(Windows version)** A nonzero value returned indicates success; zero - indicates failure. - - **(Unix version)** A zero value is returned to indicate success. An + A zero value is returned to indicate success. An exception is raised when the call failed. + .. versionchanged:: 3.8 + Previously, a nonzero value was returned on success under Windows. + .. method:: move(dest, src, count) diff --git a/Lib/test/test_mmap.py b/Lib/test/test_mmap.py index 355af8cd58935f..2c2561b523380c 100644 --- a/Lib/test/test_mmap.py +++ b/Lib/test/test_mmap.py @@ -1,4 +1,4 @@ -from test.support import (TESTFN, run_unittest, import_module, unlink, +from test.support import (TESTFN, import_module, unlink, requires, _2G, _4G, gc_collect, cpython_only) import unittest import os @@ -741,6 +741,15 @@ def test_concat_repeat_exception(self): with self.assertRaises(TypeError): m * 2 + def test_flush_return_value(self): + # mm.flush() should return zero on success, raise an + # exception on error under all platforms. + mm = mmap.mmap(-1, 16) + mm.write(b"python") + result = mm.flush() + self.assertEqual(result, 0) + mm.close() + class LargeMmapTests(unittest.TestCase): @@ -803,8 +812,5 @@ def test_around_4GB(self): self._test_around_boundary(_4G) -def test_main(): - run_unittest(MmapTests, LargeMmapTests) - if __name__ == '__main__': - test_main() + unittest.main() diff --git a/Misc/NEWS.d/next/Library/2018-08-06-21-47-03.bpo-2122.GWdmrm.rst b/Misc/NEWS.d/next/Library/2018-08-06-21-47-03.bpo-2122.GWdmrm.rst new file mode 100644 index 00000000000000..3ee381cf462cc5 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2018-08-06-21-47-03.bpo-2122.GWdmrm.rst @@ -0,0 +1,2 @@ +:meth:`mmap.mmap.flush` now returns zero on success, raises :exc:`OSError` +on error under Windows. diff --git a/Modules/mmapmodule.c b/Modules/mmapmodule.c index 27030db49b24b9..d8700e55ebe0a7 100644 --- a/Modules/mmapmodule.c +++ b/Modules/mmapmodule.c @@ -572,7 +572,11 @@ mmap_flush_method(mmap_object *self, PyObject *args) return PyLong_FromLong(0); #ifdef MS_WINDOWS - return PyLong_FromLong((long) FlushViewOfFile(self->data+offset, size)); + if (!FlushViewOfFile(self->data+offset, size)) { + PyErr_SetFromWindowsErr(GetLastError()); + return NULL; + } + return PyLong_FromLong(0); #elif defined(UNIX) /* XXX semantics of return value? */ /* XXX flags for msync? */ From 04ce1f10df72b05137281aeb17860992fe984645 Mon Sep 17 00:00:00 2001 From: Berker Peksag Date: Tue, 21 Aug 2018 16:45:32 +0300 Subject: [PATCH 2/7] address review comments --- Doc/library/mmap.rst | 8 +++++--- Doc/whatsnew/3.8.rst | 7 +++++++ Lib/test/test_mmap.py | 6 +++--- Modules/mmapmodule.c | 7 +++---- 4 files changed, 18 insertions(+), 10 deletions(-) diff --git a/Doc/library/mmap.rst b/Doc/library/mmap.rst index f4bb46820996c2..4d4556236da7ac 100644 --- a/Doc/library/mmap.rst +++ b/Doc/library/mmap.rst @@ -191,11 +191,13 @@ To map anonymous memory, -1 should be passed as the fileno along with the length changes to the given range of bytes will be flushed to disk; otherwise, the whole extent of the mapping is flushed. - A zero value is returned to indicate success. An - exception is raised when the call failed. + ``None`` is returned to indicate success. An exception is raised when the + call failed. .. versionchanged:: 3.8 - Previously, a nonzero value was returned on success under Windows. + Previously, a nonzero value was returned on success; zero was returned + on error under Windows and a zero value was returned on success; an + exception was raised on error under Unix. .. method:: move(dest, src, count) diff --git a/Doc/whatsnew/3.8.rst b/Doc/whatsnew/3.8.rst index ef5455a9f6e2a5..f5276b773dbca3 100644 --- a/Doc/whatsnew/3.8.rst +++ b/Doc/whatsnew/3.8.rst @@ -265,6 +265,13 @@ Changes in the Python API task name is visible in the ``repr()`` output of :class:`asyncio.Task` and can also be retrieved using the :meth:`~asyncio.Task.get_name` method. +* The :meth:`mmap.flush() ` method now returns ``None`` on + success and raises an exception on error under all platforms. Previously, + a nonzero value was returned on success; zero was returned on error under + Windows and a zero value was returned on success; an exception was raised on + error under Unix. + (Contributed by Berker Peksag in :issue:`2122`.) + CPython bytecode changes ------------------------ diff --git a/Lib/test/test_mmap.py b/Lib/test/test_mmap.py index 2c2561b523380c..45e7093bf50ee6 100644 --- a/Lib/test/test_mmap.py +++ b/Lib/test/test_mmap.py @@ -742,12 +742,12 @@ def test_concat_repeat_exception(self): m * 2 def test_flush_return_value(self): - # mm.flush() should return zero on success, raise an + # mm.flush() should return None on success, raise an # exception on error under all platforms. mm = mmap.mmap(-1, 16) - mm.write(b"python") + mm.write(b'python') result = mm.flush() - self.assertEqual(result, 0) + self.assertIsNone(result) mm.close() diff --git a/Modules/mmapmodule.c b/Modules/mmapmodule.c index d8700e55ebe0a7..af9cd7e2be8cc5 100644 --- a/Modules/mmapmodule.c +++ b/Modules/mmapmodule.c @@ -569,22 +569,21 @@ mmap_flush_method(mmap_object *self, PyObject *args) } if (self->access == ACCESS_READ || self->access == ACCESS_COPY) - return PyLong_FromLong(0); + Py_RETURN_NONE; #ifdef MS_WINDOWS if (!FlushViewOfFile(self->data+offset, size)) { PyErr_SetFromWindowsErr(GetLastError()); return NULL; } - return PyLong_FromLong(0); + Py_RETURN_NONE; #elif defined(UNIX) - /* XXX semantics of return value? */ /* XXX flags for msync? */ if (-1 == msync(self->data + offset, size, MS_SYNC)) { PyErr_SetFromErrno(PyExc_OSError); return NULL; } - return PyLong_FromLong(0); + Py_RETURN_NONE; #else PyErr_SetString(PyExc_ValueError, "flush not supported on this system"); return NULL; From 441b5479ab703c1007e7277563cfe141c6c3a9ae Mon Sep 17 00:00:00 2001 From: Berker Peksag Date: Tue, 21 Aug 2018 17:00:38 +0300 Subject: [PATCH 3/7] add a failed mm.flush() test --- Lib/test/test_mmap.py | 1 + 1 file changed, 1 insertion(+) diff --git a/Lib/test/test_mmap.py b/Lib/test/test_mmap.py index 45e7093bf50ee6..b160007e818233 100644 --- a/Lib/test/test_mmap.py +++ b/Lib/test/test_mmap.py @@ -748,6 +748,7 @@ def test_flush_return_value(self): mm.write(b'python') result = mm.flush() self.assertIsNone(result) + self.assertRaises(OSError, mm.flush, 1, len(b'python')) mm.close() From d4a8436c1e36126062518077a6ae7908ca818cee Mon Sep 17 00:00:00 2001 From: Berker Peksag Date: Tue, 21 Aug 2018 17:09:56 +0300 Subject: [PATCH 4/7] fix news entry --- .../next/Library/2018-08-06-21-47-03.bpo-2122.GWdmrm.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Misc/NEWS.d/next/Library/2018-08-06-21-47-03.bpo-2122.GWdmrm.rst b/Misc/NEWS.d/next/Library/2018-08-06-21-47-03.bpo-2122.GWdmrm.rst index 3ee381cf462cc5..dd31c0ec908972 100644 --- a/Misc/NEWS.d/next/Library/2018-08-06-21-47-03.bpo-2122.GWdmrm.rst +++ b/Misc/NEWS.d/next/Library/2018-08-06-21-47-03.bpo-2122.GWdmrm.rst @@ -1,2 +1,2 @@ -:meth:`mmap.mmap.flush` now returns zero on success, raises :exc:`OSError` -on error under Windows. +The :meth:`mmap.flush() ` method now returns ``None`` on +success, raises an exception on error under all platforms. From 5b1b143b79649a028a8e34f988df89df4593106f Mon Sep 17 00:00:00 2001 From: Berker Peksag Date: Tue, 21 Aug 2018 20:03:56 +0300 Subject: [PATCH 5/7] skip on windows --- Lib/test/test_mmap.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_mmap.py b/Lib/test/test_mmap.py index b160007e818233..e15fb210c13b2c 100644 --- a/Lib/test/test_mmap.py +++ b/Lib/test/test_mmap.py @@ -745,11 +745,12 @@ def test_flush_return_value(self): # mm.flush() should return None on success, raise an # exception on error under all platforms. mm = mmap.mmap(-1, 16) + self.addCleanup(mm.close) mm.write(b'python') result = mm.flush() self.assertIsNone(result) - self.assertRaises(OSError, mm.flush, 1, len(b'python')) - mm.close() + if os.name != 'nt': + self.assertRaises(OSError, mm.flush, 1, len(b'python')) class LargeMmapTests(unittest.TestCase): From 6bee75c39d0258026f0622dab9e0afedebd1f82d Mon Sep 17 00:00:00 2001 From: Berker Peksag Date: Wed, 22 Aug 2018 20:21:36 +0300 Subject: [PATCH 6/7] tweak whatsnew entry --- Doc/whatsnew/3.8.rst | 6 +++--- Lib/test/test_mmap.py | 1 + 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/Doc/whatsnew/3.8.rst b/Doc/whatsnew/3.8.rst index f5276b773dbca3..9c52325cfd8ef9 100644 --- a/Doc/whatsnew/3.8.rst +++ b/Doc/whatsnew/3.8.rst @@ -267,9 +267,9 @@ Changes in the Python API * The :meth:`mmap.flush() ` method now returns ``None`` on success and raises an exception on error under all platforms. Previously, - a nonzero value was returned on success; zero was returned on error under - Windows and a zero value was returned on success; an exception was raised on - error under Unix. + its behavior was platform-depended: a nonzero value was returned on success; + zero was returned on error under Windows. A zero value was returned on + success; an exception was raised on error under Unix. (Contributed by Berker Peksag in :issue:`2122`.) diff --git a/Lib/test/test_mmap.py b/Lib/test/test_mmap.py index e15fb210c13b2c..d513810b35be41 100644 --- a/Lib/test/test_mmap.py +++ b/Lib/test/test_mmap.py @@ -750,6 +750,7 @@ def test_flush_return_value(self): result = mm.flush() self.assertIsNone(result) if os.name != 'nt': + # 'offset' must be a multiple of mmap.PAGESIZE. self.assertRaises(OSError, mm.flush, 1, len(b'python')) From a5aa42d5de917a07cf3021c4446d340dde4fa130 Mon Sep 17 00:00:00 2001 From: Berker Peksag Date: Wed, 22 Aug 2018 20:55:30 +0300 Subject: [PATCH 7/7] more tweaks --- Doc/library/mmap.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Doc/library/mmap.rst b/Doc/library/mmap.rst index 4d4556236da7ac..c8ae7a68d37a87 100644 --- a/Doc/library/mmap.rst +++ b/Doc/library/mmap.rst @@ -191,12 +191,12 @@ To map anonymous memory, -1 should be passed as the fileno along with the length changes to the given range of bytes will be flushed to disk; otherwise, the whole extent of the mapping is flushed. - ``None`` is returned to indicate success. An exception is raised when the + ``None`` is returned to indicate success. An exception is raised when the call failed. .. versionchanged:: 3.8 Previously, a nonzero value was returned on success; zero was returned - on error under Windows and a zero value was returned on success; an + on error under Windows. A zero value was returned on success; an exception was raised on error under Unix.