GetOverlappedResult implementation #115

exvito · 2016-07-26T11:24:03Z

Sharing GetOverlappedResult implementation, which is actually very simple.

I found testing a bit more complex and banged my head around it for a while.

The key issue in the simplest test case I could devise -- based on doing an overlapped WriteFile -- was the fact that when doing overlapped IO, WriteFile (and others) set the GetLastError to ERROR_IO_PENDING and the TestCase cleanups were checking and complaining about it not being 0. All seems fine after I figured it out, validated it and SetLastError to 0 afterwards.

At first I tried going for a ReadFile based test, but I think that will require some changes to the existing ReadFile implementation: the current approach does not seem to work because it is returing a bytes object which, under overlapped IO, is not available at that time... Only after the overlapped IO completes and is waited on, for example, via GetOverlappedResult. At first sight, I'm not sure of a somewhat Pythonic approach to that. :)

Submitting PR for review and maybe some discussion around this topic.

PS: Regarding Twisted, it seems that GetOverlappedResult is used in serial port code using ReadFile/WriteFile and others so the overlapped ReadFile will need thought.

PPS: The serial code in Twisted seems to depend on pyserial which, at first glance, is using ctypes to reach out to the underlying windows APIs (good, it would be a mess for Twisted if it depended on pywin32! offtopic: maybe someday pyserial can be built on top of pywincffi, who knows)

exvito · 2016-07-26T11:55:14Z

Ooops! Something strange is going on: all tests are failing the same way, in line 49 of test_overlapped.py... Given that the assertion is failing, WriteFile is returning something other than zero which, I presume, may be indicating that overlapped IO wasn't properly triggered... :/

My envirnonment is Python 2.7 on 32 bit Windows 7 and runs the test successfuly... Could you check whether it fails or passes on yours?

PS: Any other idea as to what might be the issue here? Thanks.

opalmer · 2016-07-27T01:19:25Z

pywincffi/kernel32/overlapped.py

+
+        https://msdn.microsoft.com/en-us/library/ms683209
+
+    :param HANDLE hFile:


Need the path to the class here so it can be linked properly in the documentation.

opalmer · 2016-07-27T01:37:29Z

PPS: The serial code in Twisted seems to depend on pyserial which, at first glance, is using ctypes to reach out to the underlying windows APIs (good, it would be a mess for Twisted if it depended on pywin32! offtopic: maybe someday pyserial can be built on top of pywincffi, who knows)

When I first proposed pywincffi as the thing to replace pywin32 in Twisted I asked what else might we be able to replace and pyserial/ctypes was one the list. It's not as high as the other stuff in #69 but it's probably something worth investigating later on.

My envirnonment is Python 2.7 on 32 bit Windows 7 and runs the test successfuly... Could you check whether it fails or passes on yours? PS: Any other idea as to what might be the issue here? Thanks.

Not sure and I don't have access to my Windows VM today so I'll have to check tomorrow or Thursday depending on work. It's not clear just from the test output why this could be happening so for now I don't have a better answer. If I had to guess it might a subtle timing issue of some kind and/or have something to do with the flags being passed into the functions during the tests (WriteFile for example has a few flags specifically for overlapped I/O). I'll take a look more closely when I have access to a VM again.

Other than all that, code looks pretty good. Just had a couple of small comments but I don't see anything wrong overall with style or the major parts of the implementation. Once again, thanks for the help!

opalmer · 2016-07-30T12:38:56Z

Sorry for the delay!

So, this passes locally for me using Python 3.4 and 3.5. I expect the same results with Python 2.7 but I need to fix something in the build first:

(pywincffi-27-32) opalmer@WIN10-DEV C:\dev\repos\pywincffi
> python setup.py build
running build
running build_py
running egg_info
writing requirements to pywincffi.egg-info\requires.txt
writing pywincffi.egg-info\PKG-INFO
writing top-level names to pywincffi.egg-info\top_level.txt
writing dependency_links to pywincffi.egg-info\dependency_links.txt
reading manifest file 'pywincffi.egg-info\SOURCES.txt'
reading manifest template 'MANIFEST.in'
writing manifest file 'pywincffi.egg-info\SOURCES.txt'
running build_ext
generating cffi module 'build\\temp.win32-2.7\\Release\\_pywincffi.c'
already up-to-date
building '_pywincffi' extension
C:\Program Files (x86)\Microsoft Visual Studio 9.0\VC\BIN\cl.exe /c /nologo /Ox /MD /W3 /GS- /DNDEBUG -DUNICODE=1 -D_UNICODE=1 -Ic:\python27-x86\include -IC:\dev\virtualenv\pywincffi-27-32\PC /Tcbuild\temp.win32-2.7\Release\_pywincffi.c /Fobuild\temp.win32-2.7\Release\build\temp.win32-2.7\Release\_pywincffi.obj
_pywincffi.c
build\temp.win32-2.7\Release\_pywincffi.c(3653) : error C2065: 'INHERIT_PARENT_AFFINITY' : undeclared identifier
build\temp.win32-2.7\Release\_pywincffi.c(3654) : error C2065: 'INHERIT_PARENT_AFFINITY' : undeclared identifier
error: command 'C:\\Program Files (x86)\\Microsoft Visual Studio 9.0\\VC\\BIN\\cl.exe' failed with exit status 2

opalmer · 2016-07-30T13:25:02Z

Merged #117, your tests work fine in Python 2.7 too so I'm still not sure exactly why this fails on AppVeyor.

exvito · 2016-07-30T22:52:13Z

(quick feedback)

Thanks for your review and comments. I've been very busy these days as I'm off for a one week vacation... Hopefully I'll be able do some improvements tomorrow morning. Otherwise I'll only be able to get back to this in about two weeks, I guess. :/

codecov-io · 2016-07-31T13:16:22Z

Current coverage is 98.98% (diff: 100%)

Merging #115 into master will increase coverage by 0.01%

Powered by Codecov. Last update 1b39e7b...4842a58

exvito · 2016-07-31T13:20:14Z

Progress:

Updated docstrings + code as per your review.
Tests now passing on AppVeyor - Per MSDN docs, WriteFile can return success/failure and GetLastError() can be SUCCESS/ERROR_IO_PENDING...
Something is failing on one of the checks, but I'm not sure I understand it.

Any pointers?

opalmer · 2016-07-31T13:25:59Z

tests/test_kernel32/test_overlapped.py

+
+        _ = WriteFile(handle, file_contents, lpOverlapped=ovr)
+        error_code, _ = self.GetLastError()
+        self.assertIn(error_code, (lib.ERROR_IO_PENDING, 0))


Interestingly, I merged some code yesterday you could use here:

https://github.com/opalmer/pywincffi/blob/master/pywincffi/dev/testutil.py#L295

Basically, replace assertIn and SetLastError with:

self.maybe_assert_last_error(lib.ERROR_IO_PENDING)

opalmer · 2016-07-31T13:30:38Z

The one check is failing because you're missing some coverage in overlapped.py:

https://codecov.io/gh/opalmer/pywincffi/pull/115/compare

If possible, it would be nice to have a test which covers that code. You could probably use mock_library from testutil to replace library.GetOverlappedResult with another function that calls SetLastError internally and returns an exit code of zero. That should hit on the if statement you have under except WindowsAPIError.

exvito · 2016-07-31T17:54:58Z

Two questions before proceeding:

To use self.maybe_assert_last_error(lib.ERROR_IO_PENDING) I'll have to merge current master into this branch. Is that ok by you? Any other suggestion?
In reviewing the code around the last error_check I'm really not sure why the try/except is there... Maybe I copied it from some other function and left it alone without giving it much thought. From MSDN's docs, GetOverlappedResult should simply return NON_ZERO on success. Won't the correct implementation be simply the error_check with no try/except?

opalmer · 2016-07-31T20:08:34Z

To use self.maybe_assert_last_error(lib.ERROR_IO_PENDING) I'll have to merge current master into this branch. Is that ok by you? Any other suggestion?

Merging from the current master is fine.

Won't the correct implementation be simply the error_check with no try/except?

Correct yes. Sorry I should have caught this in the review...

exvito · 2016-08-01T08:27:40Z

I think this is ready for merging -- pending your review, of course.
Next would be:

Update ReadFile to support overlapped I/O as discussed in ReadFile - Change to support overlapped IO #116.
Update GetOverlappedResult tests to test for overlapped ReadFile.

PS: I'll be for a couple of weeks on vacation. :)

opalmer · 2016-08-01T22:29:31Z

I think this is ready for merging -- pending your review, of course.

Reviewed and I agree, merging.

Next would be:

I'll try to put some time into these if I have the time over the next couple of weeks.

PS: I'll be for a couple of weeks on vacation. :)

Enjoy your vacation and thanks again!

exvito added 7 commits July 24, 2016 11:47

Initial code for GetOverlappedResult

c02d634

Fix WriteFile result checking in the async case

a1757dd

Preliminary work on GetOverlappedResult test

bc61e17

Fix internal CFFI/C type in GetOverlappedResult

3cd2e6d

lint - Missing final newline in kernel32/__init__.py

7fc4b28

One GetOverlappedResult test in place - WriteFile

dc9f0bc

GetOverlappedResult - Fix silly text/bytes conversion mistake

cd84b6b

exvito mentioned this pull request Jul 26, 2016

ReadFile - Change to support overlapped IO #116

Open

opalmer added this to the 0.4.0 milestone Jul 27, 2016

opalmer added enhancement lib: kernel32 labels Jul 27, 2016

opalmer mentioned this pull request Jul 27, 2016

High Priority Windows API Functions, Structures and Constants #69

Closed

37 tasks

opalmer reviewed Jul 27, 2016
View reviewed changes

opalmer closed this Jul 30, 2016

opalmer reopened this Jul 30, 2016

exvito added 2 commits July 31, 2016 13:21

GetOverlappedResult adjustments following review

57d80a2

Fix GetOverlappedResult WriteFile tests per MSDN docs

9cd217e

opalmer reviewed Jul 31, 2016
View reviewed changes

exvito added 3 commits August 1, 2016 08:49

Merge remote-tracking branch 'upstream/master' into get-ov-result

9635977

Improved GetOverlappedResult error_check and WriteFile test

2288d5d

Removed unused import in GetOverlappedResult wrapper

4842a58

opalmer merged commit 3a43d36 into opalmer:master Aug 1, 2016

opalmer added a commit that referenced this pull request Aug 1, 2016

adding changelog entry for #115

3428287

opalmer mentioned this pull request Aug 1, 2016

GetOverlappedResult implementation: changelog #119

Merged

opalmer added a commit that referenced this pull request Aug 1, 2016

adding changelog entry for #115 (#119)

7269987

exvito deleted the get-ov-result branch August 13, 2016 10:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GetOverlappedResult implementation #115

GetOverlappedResult implementation #115

exvito commented Jul 26, 2016

exvito commented Jul 26, 2016

opalmer Jul 27, 2016

opalmer commented Jul 27, 2016

opalmer commented Jul 30, 2016

opalmer commented Jul 30, 2016 •

edited

Loading

exvito commented Jul 30, 2016

codecov-io commented Jul 31, 2016 •

edited

Loading

exvito commented Jul 31, 2016

opalmer Jul 31, 2016 •

edited

Loading

opalmer commented Jul 31, 2016

exvito commented Jul 31, 2016

opalmer commented Jul 31, 2016

exvito commented Aug 1, 2016

opalmer commented Aug 1, 2016


		https://msdn.microsoft.com/en-us/library/ms683209

		:param HANDLE hFile:

GetOverlappedResult implementation #115

GetOverlappedResult implementation #115

Conversation

exvito commented Jul 26, 2016

exvito commented Jul 26, 2016

opalmer Jul 27, 2016

Choose a reason for hiding this comment

opalmer commented Jul 27, 2016

opalmer commented Jul 30, 2016

opalmer commented Jul 30, 2016 • edited Loading

exvito commented Jul 30, 2016

codecov-io commented Jul 31, 2016 • edited Loading

Current coverage is 98.98% (diff: 100%)

exvito commented Jul 31, 2016

opalmer Jul 31, 2016 • edited Loading

Choose a reason for hiding this comment

opalmer commented Jul 31, 2016

exvito commented Jul 31, 2016

opalmer commented Jul 31, 2016

exvito commented Aug 1, 2016

opalmer commented Aug 1, 2016

opalmer commented Jul 30, 2016 •

edited

Loading

codecov-io commented Jul 31, 2016 •

edited

Loading

opalmer Jul 31, 2016 •

edited

Loading