Hi Sebastian,
On 04/26/16 16:11, Sebastian Lackner wrote:
On 26.04.2016 14:30, Jacek Caban wrote:
It's a nice way to force async read, but when you add tests with predictable reads it would be nice to take more advantage of that. What do you think about:
- call InternetReadFileEx and check that it's done sync
- call InternetReadFileEx and check that it's done async
- set conn_wait_event, wait completion and make sure we read all
remaining data
- call InternetReadFileEx and make sure it synchronously hits the end of
data
Thanks, Jacek
I don't think its necessary to restructure the whole code, unrolling the loop would require a lot of code duplication because Wine still behaves different in many aspects, and even on Windows it depends on things like buffer sizes.
I expected Wine to be closer to Windows in this case. I experimented a bit with your tests and I can see that those are less predictable than I thought. You'd see Wine behaviour by using IRF_NO_WAIT, so handling flags 0 is a problem in Wine, but not the only one. Anyway, those are unrelated to your changes, so it's fine as is.
I've added more tests (some as todo_wine) for the things you pointed out below though. Are you fine with the new patch, or any other cases you would like to have tested?
Thanks for adding them. Just a few comments:
+ if (!pending_reads++) + { + res = WaitForSingleObject( hCompleteEvent, 200 ); + ok( res == WAIT_TIMEOUT, "expected WAIT_TIMEOUT, got %u\n", res ); + SetEvent( conn_wait_event ); + }
I'm not sure what is this for and I'd like to avoid adding expected timeouts is possible. Checking last error ERROR_IO_PENDING seems enough to consider it being done asynchronously. Sorry if you misunderstood my comment about checking it for being async.
+ todo_wine_if( ib.dwBufferLength == 0 ) + ok( ib.dwBufferLength != 0, "expected ib.dwBufferLength != 0\n" );
todo_wine_if(pending_reads > 1) should do the trick here and won't completely disable the test on Wine.
Same comments apply to InternetReadFile tests.
Thanks, Jacek