[Python-checkins] bpo-2122: Make mmap.flush() behave same on all platforms (GH-8692)

[Berker Peksag]

https://github.com/python/cpython/commit/e7d4b2f205c711d056bea73a0093e2e2b200544b
commit: e7d4b2f205c711d056bea73a0093e2e2b200544b
branch: master
author: Berker Peksag <berker.peksag at gmail.com>
committer: GitHub <noreply at github.com>
date: 2018-08-22T21:21:05+03:00
summary:

bpo-2122: Make mmap.flush() behave same on all platforms (GH-8692)

Previously, its behavior was platform-dependent and there was no error checking
under Windows.

files:
A Misc/NEWS.d/next/Library/2018-08-06-21-47-03.bpo-2122.GWdmrm.rst
M Doc/library/mmap.rst
M Doc/whatsnew/3.8.rst
M Lib/test/test_mmap.py
M Modules/mmapmodule.c

diff --git a/Doc/library/mmap.rst b/Doc/library/mmap.rst
index ca09a6a3ca99..c8ae7a68d37a 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.
 
-      **(Windows version)** A nonzero value returned indicates success; zero
-      indicates failure.
+      ``None`` is returned to indicate success.  An exception is raised when the
+      call failed.
 
-      **(Unix version)** 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; zero was returned
+         on error under Windows.  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 1f816a1436e8..d07896b71f23 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() <mmap.mmap.flush>` method now returns ``None`` on
+  success and raises an exception on error under all platforms.  Previously,
+  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`.)
+
 
 CPython bytecode changes
 ------------------------
diff --git a/Lib/test/test_mmap.py b/Lib/test/test_mmap.py
index 355af8cd5893..d513810b35be 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,18 @@ def test_concat_repeat_exception(self):
         with self.assertRaises(TypeError):
             m * 2
 
+    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)
+        if os.name != 'nt':
+            # 'offset' must be a multiple of mmap.PAGESIZE.
+            self.assertRaises(OSError, mm.flush, 1, len(b'python'))
+
 
 class LargeMmapTests(unittest.TestCase):
 
@@ -803,8 +815,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 000000000000..dd31c0ec9089
--- /dev/null
+++ b/Misc/NEWS.d/next/Library/2018-08-06-21-47-03.bpo-2122.GWdmrm.rst
@@ -0,0 +1,2 @@
+The :meth:`mmap.flush() <mmap.mmap.flush>` method now returns ``None`` on
+success, raises an exception on error under all platforms.
diff --git a/Modules/mmapmodule.c b/Modules/mmapmodule.c
index 27030db49b24..af9cd7e2be8c 100644
--- a/Modules/mmapmodule.c
+++ b/Modules/mmapmodule.c
@@ -569,18 +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
-    return PyLong_FromLong((long) FlushViewOfFile(self->data+offset, size));
+    if (!FlushViewOfFile(self->data+offset, size)) {
+        PyErr_SetFromWindowsErr(GetLastError());
+        return NULL;
+    }
+    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;