Python 3.3 has _winapi.Overlapped. Python 3.4 _winapi.Overlapped but also _overlapped.Overlapped. Why do we have two implementations?

Most code looks to be duplicated.

I found this issue while I was trying to understand the issue of pending operations which are not cancelled. The issue #19565 changed the behaviour in the _winapi module, but not in the _overlapped module :-(

New changeset da10196b94f4 by Richard Oudkerk in branch 'default':
Issue #19565: Prevent warnings at shutdown about pending overlapped ops.
http://hg.python.org/cpython/rev/da10196b94f4

We know have two implementations. Which one is the best? I'm trying to understand why Python may crash on Windows with asyncio when the proactor event loop is used.

_winapi is used for example by multiprocessing, whereas _overlapped is used by asyncio.

I think overlapped.c should be merged back to _winapi (apparently, it started with code from _winapi). overlapped has a number of functions that aren't in _winapi, but should be (IMO). Then, the _overlapped module should be dropped.

Of course, this may break code synchronization with Tulip. Guido: is it ok to proceed in this direction?

One sub issue then is naming: _overlapped renamed Overlapped.GetOverlappedResult to Overlapped.getresult. I think the _winapi name is better, since all other methods also use Windows API function names.

> One sub issue then is naming: _overlapped renamed Overlapped.GetOverlappedResult to Overlapped.getresult. I think the _winapi name is better, since all other methods also use Windows API function names.

I also prefer Overlapped.GetOverlappedResult name.

There are other differences. Using _overlapped, the overlapped object is created before calling an operation like WriteFile() or AcceptEx() (ex: ov.WriteFile(...)). Using _winapi, the operation may create an overlapped object if asked (ex: WriteFile(..., overlapped=True)).

The structure is different.

_winapi.c:
---
typedef struct {
    PyObject_HEAD
    OVERLAPPED overlapped;
    /* For convenience, we store the file handle too */
    HANDLE handle;
    /* Whether there's I/O in flight */
    int pending;
    /* Whether I/O completed successfully */
    int completed;
    /* Buffer used for reading (optional) */
    PyObject *read_buffer;
    /* Buffer used for writing (optional) */
    Py_buffer write_buffer;
} OverlappedObject;
---

overlapped.c:
---
enum {TYPE_NONE, TYPE_NOT_STARTED, TYPE_READ, TYPE_WRITE, TYPE_ACCEPT,
      TYPE_CONNECT, TYPE_DISCONNECT, TYPE_CONNECT_NAMED_PIPE,
      TYPE_WAIT_NAMED_PIPE_AND_CONNECT};

typedef struct {
    PyObject_HEAD
    OVERLAPPED overlapped;
    /* For convenience, we store the file handle too */
    HANDLE handle;
    /* Error returned by last method call */
    DWORD error;
    /* Type of operation */
    DWORD type;
    union {
        /* Buffer used for reading (optional) */
        PyObject *read_buffer;
        /* Buffer used for writing (optional) */
        Py_buffer write_buffer;
    };
} OverlappedObject;
---

And the object in overlapped.c has much more methods.

_winapi.c:
---
static PyMethodDef overlapped_methods[] = {
    {"GetOverlappedResult", (PyCFunction) overlapped_GetOverlappedResult,
                            METH_O, NULL},
    {"getbuffer", (PyCFunction) overlapped_getbuffer, METH_NOARGS, NULL},
    {"cancel", (PyCFunction) overlapped_cancel, METH_NOARGS, NULL},
    {NULL}
};
---

overlapped.c:
---
static PyMethodDef Overlapped_methods[] = {
    {"getresult", (PyCFunction) Overlapped_getresult,
     METH_VARARGS, Overlapped_getresult_doc},
    {"cancel", (PyCFunction) Overlapped_cancel,
     METH_NOARGS, Overlapped_cancel_doc},
    {"ReadFile", (PyCFunction) Overlapped_ReadFile,
     METH_VARARGS, Overlapped_ReadFile_doc},
    {"WSARecv", (PyCFunction) Overlapped_WSARecv,
     METH_VARARGS, Overlapped_WSARecv_doc},
    {"WriteFile", (PyCFunction) Overlapped_WriteFile,
     METH_VARARGS, Overlapped_WriteFile_doc},
    {"WSASend", (PyCFunction) Overlapped_WSASend,
     METH_VARARGS, Overlapped_WSASend_doc},
    {"AcceptEx", (PyCFunction) Overlapped_AcceptEx,
     METH_VARARGS, Overlapped_AcceptEx_doc},
    {"ConnectEx", (PyCFunction) Overlapped_ConnectEx,
     METH_VARARGS, Overlapped_ConnectEx_doc},
    {"DisconnectEx", (PyCFunction) Overlapped_DisconnectEx,
     METH_VARARGS, Overlapped_DisconnectEx_doc},
    {"ConnectNamedPipe", (PyCFunction) Overlapped_ConnectNamedPipe,
     METH_VARARGS, Overlapped_ConnectNamedPipe_doc},
    {"WaitNamedPipeAndConnect",
     (PyCFunction) Overlapped_WaitNamedPipeAndConnect,
     METH_VARARGS, Overlapped_WaitNamedPipeAndConnect_doc},
    {NULL}
};
---

_winapi.c doesn't provide AcceptEx() nor Accept().

_overlapped is linked against the socket library whereas _winapi is not so
it can be bundled in with python3.dll.

I did intend to switch multiprocessing over to using _overlapped but I did
not get round to it.

Since this is a private module the names of methods do not matter to much.
Note that getresult and GetOverlappedResult return values in different
forms.

> I did intend to switch multiprocessing over to using _overlapped
> but I did not get round to it.

Do you mean that _overlapped module is newer and should be used instead of _winapi? If multiprocssing is patched to use _overlapped, we can drop overlapped code from _winapi, should we keep functions like WriteFile() without overlapped support? (I think that we should keep these functions, it was discussed to support the native Windows API for files.)

IMO such change can be done in Python 3.5, it is risky and can wait.

But until that, I'm concerned by overlapped deallocator which is different in the two modules. Attached fixes _overlapped module to use the same logic than _winapi: give up on Windows XP during Python finalization if the overlapped is still pending, don't deallocate memory, exit immediatly. See issue #19565 for the rationale of this change.

It seems fine to me to link pythonXY.dll with wsock32.dll. This is a standard library in Windows NT+, so there is no need to avoid linking with it.

I also now agree that any change that we may make is too big for 3.4, so I propose to defer any action on this issue to 3.5.

I'm fine with this refactoring. The upstream tulip/asyncio repo will keep its own _overlapped for the purpose of supporting Python 3.3. (I'm not sure what Victor wants to do with Trollius but probably the same.)

Can please someone review overlapped_dealloc.patch? I would like to apply it on Python 3.4.

See my review in Rietveld.

> See my review in Rietveld.

Thanks Martin, but I cannot answer to your question. I didn't write this part of the code and I don't know it. Maybe Richard or Antoine would be able to answer?

Sorry Martin, I'm not sure that I understood your proposition. Here is a new patch.

@Martin, Richard: Could you please review overlapped_dealloc-2.patch? It should be applied on Python 3.4 IMO.

What bad thing will happen if I don't accept this for 3.4?

It cannot be applied to 3.4.0 without a test case. Whether it can be applied to 3.4.0 with a test case, I don't know - IMO *nothing* can be applied anymore, unless it is release-critical (e.g. Python crashes when calling print()). Bug fixes go into 3.4.1 (and I still think this would need a test case).

Also, notice that your patches do not address this issue ("Python 3.4 has two Overlapped types").

Larry, I think this will happen (Victor, correct me if I'm wrong):

On Windows XP only, if this overlapped object is used and not properly released before the process terminates, python may currently crash at the end of the process, and it will announce that this might happen.

On Windows Vista+, the system resources (OVERLAPPED) is properly released when the Python object is released, but on XP, the functionality is not available. The patch works around by closing the handle that would otherwise cause the interpreter crash.

The original complaint that triggered this fix in the other place was not that the process crashed, but that Python announced that it would.

follow-up: the work around is not the closing of the handle, but leaving the buffer allocated (as a memory leak), so that Windows can still write into it if it wants to (which it may or may not chose to do).

Some questions.

a) Is at least one of these Overlapped objects new in 3.4?
b) Which of these Overlapped objects is used by asyncio?
c) Is the goal of merging the two objects simple code hygiene, or is the merged object going to work better somehow?  Like, will it work correctly on XP and Vista, and always shut down properly?

I think this is all much too late for 3.4.

Martin> Larry, I think this will happen (Victor, correct me if I'm wrong):

Sorry but I didn't write this code and I'm not sure that I understand it. It's just surprising that a bug was fixed in one file but not the other one, and if I remember correctly, the patch fixed a crash.

Larry> a) Is at least one of these Overlapped objects new in 3.4?

The _overlapped module is new and was written for asyncio. It adds new features for sockets.

Larry> b) Which of these Overlapped objects is used by asyncio?

asyncio uses _overlapped, multiprocessing uses _winapi.

Larry> c) Is the goal of merging the two objects simple code hygiene, or is the merged object going to work better somehow?

I proposed to merge the code to reduce the number of lines and make the maintenance simpler. For example, a bug was recently fixed in a module but not in the other one.

Antoine> I think this is all much too late for 3.4.

I didn't propose to merge duplicated code in Python 3.4 but 3.5. Antoine, just to be sure, are you talking about merging duplicated code or applying  overlapped_dealloc-2.patch on Python 3.4?

> Antoine> I think this is all much too late for 3.4.
> 
> I didn't propose to merge duplicated code in Python 3.4 but 3.5.
> Antoine, just to be sure, are you talking about merging duplicated
> code or applying  overlapped_dealloc-2.patch on Python 3.4?

Both. Is overlapped_dealloc-2.patch fixing something important?

Oh, I agree that refactoring is too late for Python 3.4. Anyway, I thought that the two modules had just duplicated code, but in fact they also have a different API. So it's not so easy to merge them. It would probably be easier if asyncio and multiprocessing use the same module.

> Is overlapped_dealloc-2.patch fixing something important?

overlapped_dealloc.patch was just the same change from _winapi to _overlapped to fix #19565.

Richard fixed #19565, not me.

Martin asked me to change overlapped_dealloc.patch but I'm not sure that I understood his request, that why I asked for a review of  overlapped_dealloc-2.patch.

I don't know if it's important to fix #19565 in the _overlapped module. _winapi has a test of overlapped operations still running at Python exit, but _overlapped doesn't have such test yet. With asyncio is may become more likely to exit with pending overlapped operations.

Victor, you really shouldn't submit code that you don't understand. If you do, please don't claim that including it is urgent :-(. Based on that ground, I suggest to

a) postpone this detail to 3.4.1, and
b) create a new issue for this bug fix, and
c) leave this issue for the merging of the two implementations, in 3.5.

> Victor, you really shouldn't submit code that you don't understand.

In fact I do understand overlapped_dealloc.patch, it's "quite simple". On Windows XP, if you have exit Python with pending overlapped operations, you must not release the read or write buffer of the operation, or the process may crash because Windows tries to read from or write into freed memory. The trick is to not release the read/write buffer (but close the Windows handle of the overlapped operation) if the operation is pending and python is exiting.

On more recent version, you can safetly cancel a pending operations, so the issue is specific to Windows XP (and older, but we don't support Windows 2000 anymore, right?).

Martin: I didn't understand your suggestion on Rietveld:
"Why is this the default case? I think it should be the ERROR_IO_INCOMPLETE case, because only this error handled. Any other error should IMO be raised as is, and the resources still be released."

I wrote a patch just to ask you if it is the change you suggested, and you still didn't answer to this question ("is overlapped_dealloc-2.patch what you wanted?")

I'm not sure that it is safe to release the memory if the state is different than ERROR_IO_INCOMPLETE. Well, it is safer to never release memory at exit, but it may leeak memory. So maybe Martin's suggestion can be delayed after Python 3.4 (to give more time to buildbots to test it), but Richard's fix (overlapped_dealloc.patch) can be applied (on _overlapped) to Python 3.4?

--

Sorry for mixing two different issues in the same issue, in fact  overlapped_dealloc.patch is just the follow up of #19565 for the _overlapped module. I didn't expect so long discussions for such patch :-(

I fail to see the relevant difference between overlapped_dealloc.patch and overlapped_dealloc-2.patch. What I was asking was that

   default:

be replaced with

  case ERROR_IO_INCOMPLETE:

and a new

  default:

be added that handles all other errors.