classification
Title: Python 3.4 has two Overlapped types
Type: Stage: resolved
Components: Windows Versions: Python 3.5
process
Status: closed Resolution: out of date
Dependencies: Superseder:
Assigned To: Nosy List: gvanrossum, larry, loewis, pitrou, sbt, vstinner, zach.ware
Priority: normal Keywords: patch

Created on 2014-01-28 01:51 by vstinner, last changed 2018-09-19 23:15 by vstinner. This issue is now closed.

Files
File name Uploaded Description Edit
overlapped_dealloc.patch vstinner, 2014-01-28 08:50 review
overlapped_dealloc-2.patch vstinner, 2014-02-03 01:21 review
Messages (27)
msg209496 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-01-28 01:51
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.
msg209497 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-01-28 01:54
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.
msg209498 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-01-28 01:56
_winapi is used for example by multiprocessing, whereas _overlapped is used by asyncio.
msg209505 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2014-01-28 07:20
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?
msg209506 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2014-01-28 07:27
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.
msg209512 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-01-28 08:31
> 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().
msg209513 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2014-01-28 08:39
_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.
msg209515 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-01-28 08:50
> 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.
msg209540 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2014-01-28 12:43
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.
msg209556 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2014-01-28 15:39
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.)
msg209562 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-01-28 16:31
Can please someone review overlapped_dealloc.patch? I would like to apply it on Python 3.4.
msg209578 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2014-01-28 19:32
See my review in Rietveld.
msg209587 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-01-28 21:29
> 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?
msg210068 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-02-03 01:21
Sorry Martin, I'm not sure that I understood your proposition. Here is a new patch.
msg210988 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-02-11 17:44
@Martin, Richard: Could you please review overlapped_dealloc-2.patch? It should be applied on Python 3.4 IMO.
msg211005 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-02-11 19:31
What bad thing will happen if I don't accept this for 3.4?
msg211010 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2014-02-11 20:28
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").
msg211016 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2014-02-11 20:44
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.
msg211018 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2014-02-11 20:47
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).
msg211020 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-02-11 20:56
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?
msg211022 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-02-11 21:01
I think this is all much too late for 3.4.
msg211033 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-02-11 23:06
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?
msg211035 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-02-11 23:10
> 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?
msg211038 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-02-11 23:22
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.
msg211039 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2014-02-11 23:28
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.
msg211045 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-02-11 23:54
> 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 :-(
msg211334 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2014-02-16 18:12
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.
History
Date User Action Args
2018-09-19 23:15:29vstinnersetstatus: open -> closed
resolution: out of date
stage: resolved
2014-02-16 18:12:07loewissetmessages: + msg211334
2014-02-11 23:54:36vstinnersetmessages: + msg211045
versions: - Python 3.4
2014-02-11 23:28:12loewissetmessages: + msg211039
2014-02-11 23:22:53vstinnersetmessages: + msg211038
2014-02-11 23:10:54pitrousetmessages: + msg211035
2014-02-11 23:06:26vstinnersetmessages: + msg211033
2014-02-11 21:01:45pitrousetmessages: + msg211022
2014-02-11 20:56:21larrysetmessages: + msg211020
2014-02-11 20:47:43loewissetmessages: + msg211018
2014-02-11 20:44:46loewissetmessages: + msg211016
2014-02-11 20:28:10loewissetmessages: + msg211010
2014-02-11 19:31:38larrysetmessages: + msg211005
2014-02-11 17:53:19pitrousetnosy: + larry
2014-02-11 17:44:41vstinnersetmessages: + msg210988
versions: + Python 3.4
2014-02-03 01:21:32vstinnersetfiles: + overlapped_dealloc-2.patch

messages: + msg210068
2014-01-28 21:29:28vstinnersetmessages: + msg209587
2014-01-28 19:32:57loewissetmessages: + msg209578
2014-01-28 16:31:43vstinnersetmessages: + msg209562
2014-01-28 15:39:16gvanrossumsetmessages: + msg209556
2014-01-28 15:12:16zach.waresetnosy: + zach.ware
2014-01-28 12:43:56loewissetmessages: + msg209540
versions: + Python 3.5, - Python 3.4
2014-01-28 08:50:23vstinnersetfiles: + overlapped_dealloc.patch
keywords: + patch
messages: + msg209515
2014-01-28 08:39:16sbtsetmessages: + msg209513
2014-01-28 08:31:57vstinnersetmessages: + msg209512
2014-01-28 07:27:23loewissetmessages: + msg209506
2014-01-28 07:20:56loewissetnosy: + loewis
messages: + msg209505
2014-01-28 01:56:53vstinnersetmessages: + msg209498
2014-01-28 01:54:59vstinnersetmessages: + msg209497
2014-01-28 01:51:53vstinnercreate