mmap.flush does not check for errors on windows

[b510eab9-9bb7-4789-a06f-c0a1898e20d3]

BPO	2122
Nosy	@pfmoore, @atsuoishimoto, @pitrou, @tjguk, @berkerpeksag, @zware, @serhiy-storchaka, @zooba
PRs	python/cpython#8692
Files	mmap-flush.txt: make flush return None

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields: ```python assignee = None closed_at = created_at = labels = ['3.8', 'type-bug', 'library'] title = 'mmap.flush does not check for errors on windows' updated_at = user = 'https://bugs.python.org/schmir' ``` bugs.python.org fields: ```python activity = actor = 'berker.peksag' assignee = 'none' closed = True closed_date = closer = 'berker.peksag' components = ['Library (Lib)'] creation = creator = 'schmir' dependencies = [] files = ['9645'] hgrepos = [] issue_num = 2122 keywords = ['patch'] message_count = 11.0 messages = ['62427', '63436', '65549', '65550', '65551', '138227', '242715', '323563', '323571', '323898', '323899'] nosy_count = 10.0 nosy_names = ['paul.moore', 'ishimoto', 'pitrou', 'ocean-city', 'schmir', 'tim.golden', 'berker.peksag', 'zach.ware', 'serhiy.storchaka', 'steve.dower'] pr_nums = ['8692'] priority = 'normal' resolution = 'fixed' stage = 'resolved' status = 'closed' superseder = None type = 'behavior' url = 'https://bugs.python.org/issue2122' versions = ['Python 3.8'] ```

[b510eab9-9bb7-4789-a06f-c0a1898e20d3]

mmap.flush returns the result of the call to FlushViewOfFile as an integer, and does not check for errors. On unix it does check for errors. The function should return None and raise an exception if an error occurs... This bug can lead to data loss...

Here's the current version of that function:

static PyObject *
mmap_flush_method(mmap_object *self, PyObject *args)
{
    Py_ssize_t offset = 0;
    Py_ssize_t size = self->size;
    CHECK_VALID(NULL);
    if (!PyArg_ParseTuple(args, "|nn:flush", &offset, &size))
        return NULL;
    if ((size_t)(offset + size) > self->size) {
        PyErr_SetString(PyExc_ValueError, "flush values out of range");
        return NULL;
    }
#ifdef MS_WINDOWS
    return PyInt_FromLong((long) FlushViewOfFile(self->data+offset, size));
#elif defined(UNIX)
    /* XXX semantics of return value? */
    /* XXX flags for msync? */
    if (-1 == msync(self->data + offset, size, MS_SYNC)) {
        PyErr_SetFromErrno(mmap_module_error);
        return NULL;
    }
    return PyInt_FromLong(0);
#else
    PyErr_SetString(PyExc_ValueError, "flush not supported on this system");
    return NULL;
#endif
}

[b510eab9-9bb7-4789-a06f-c0a1898e20d3]

attached is a patch. I hope it is ok to change the semantics a bit. They were very strange:

• on unix it raises an exception on errors. otherwise it always returns 0.
• on windows it returns non-zero on success, and returns zero on errors.

Now, flush returns None or raises an Exception.

[b510eab9-9bb7-4789-a06f-c0a1898e20d3]

now this insanity is even documented with svn revision 62359 (http://hgpy.de/py/trunk/rev/442252bce780)

[pitrou]

Perhaps it would be better if the patch came with unit tests. I agree that the situation right now is insane.

PS : I have no power to commit or even accept a patch :)

[b510eab9-9bb7-4789-a06f-c0a1898e20d3]

I thought about this too, but I don't know of a way to provoke an error. (Other than passing in illegal values, but the code tries to catch those. And btw, raises an Exception on windows :)

One could currently pass in a negative value for size (this isn't caught in the checks). e.g.

m.flush(500, -500) gives an 'error: [Errno 22] Invalid argument' on linux.

but then you don't want to rely on another bug for testing purposes.

[a15f0e1c-8505-401e-a0e3-653e6d54b44d]

This issue seems to be reproduced in following way.

1. Attach USB flash drive. (On my machine, it was attached as E drive)

2. Run python interactive shell and run following commands. (Confirmed on Python2.6)

   > import mmap > f = open("e:/temp.tmp", "w+b") > f.write("foo") > f.flush() > m = mmap.mmap(f.fileno(), 3) > m[:] = "xxx"

3. Detach USB flash drive violently here! (Without safety mechanism to detach USB flash drive, just plug it off) Note that python shell is still running.

4. Enter following command in python shell. > m.flush() It returns *0*. (Means failure)

[83d2e70e-e599-4a04-b820-3814bbdb9bef]

I think we should be properly handling errors. If people agree I'll provide a new patch to cover code and doc changes, but I've no idea how to provide any form of unit test for the change.

[serhiy-storchaka]

If we break compatibility in any case, what if return None instead of 0? The function always returning 0 looks strange. The function always returning None is common.

[zooba]

I agree. We should definitely document it as a change, but I don't think there's anything useful you can do in the case of a flush failing anyway, so it seems unlikely that anyone could depend on the return value.

Returning None for success and exception on error sounds like a good change. Technically this is no longer returning zero, as previously documented, so if anyone wants to get *really* pedantic, we're not returning the "failed" result for success ;)

[berkerpeksag]

New changeset e7d4b2f205c711d056bea73a0093e2e2b200544b by Berker Peksag in branch 'master': bpo-2122: Make mmap.flush() behave same on all platforms (GH-8692) https://github.com/python/cpython/commit/e7d4b2f205c711d056bea73a0093e2e2b200544b

[berkerpeksag]

Thanks for the suggestions and for the reviews!