Re: [2/6] wininet/tests: Add tests for asynchronous InternetReadFileEx.
Hi Sebastian, On 04/25/16 21:41, Sebastian Lackner wrote:
+ memset( &ib, 0, sizeof(ib) ); + memset( buffer, 0, sizeof(buffer) ); + ib.dwStructSize = sizeof(ib); + for (count = 0; count < sizeof(buffer); count += ib.dwBufferLength) + { + ib.lpvBuffer = buffer + count; + ib.dwBufferLength = sizeof(buffer) - count; + + SET_OPTIONAL( INTERNET_STATUS_RECEIVING_RESPONSE ); + SET_OPTIONAL( INTERNET_STATUS_RESPONSE_RECEIVED ); + + ret = InternetReadFileExA( req, &ib, 0, 0xdeadbeef ); + if (!ret) + { + ok( GetLastError() == ERROR_IO_PENDING, "expected ERROR_IO_PENDING, got %u\n", GetLastError() ); + SET_EXPECT( INTERNET_STATUS_REQUEST_COMPLETE ); + SetEvent( conn_wait_event ); + WaitForSingleObject( hCompleteEvent, INFINITE ); + ok( req_error == ERROR_SUCCESS, "expected ERROR_SUCCESS, got %u\n", req_error ); + CHECK_NOTIFIED( INTERNET_STATUS_REQUEST_COMPLETE ); + }
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
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'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? Regards, Sebastian
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
On 26.04.2016 22:47, Jacek Caban wrote:
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.
Yes, thats also what I noticed when I played around with it. We can probably make the tests more precise at some later point.
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.
The idea of this additional check was to ensure that the request cannot be fulfilled with the current buffer contents. Otherwise its not completely clear that async handler is really waiting for new data, as it should be. If you prefer, I can also remove this test again or set the timeout to 0 to avoid blocking. As the tests show, this part is already working fine. ;)
+ 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.
Okay, I will change that.
Same comments apply to InternetReadFile tests.
Thanks, Jacek
On 04/26/16 23:00, Sebastian Lackner wrote:
The idea of this additional check was to ensure that the request cannot be fulfilled with the current buffer contents. Otherwise its not completely clear that async handler is really waiting for new data, as it should be. If you prefer, I can also remove this test again or set the timeout to 0 to avoid blocking. As the tests show, this part is already working fine. ;)
Yes, please. Both timeout 0 and removing the test is fine with me. BTW, other patches look good to me. Thanks, Jacek
On 26.04.2016 23:13, Jacek Caban wrote:
On 04/26/16 23:00, Sebastian Lackner wrote:
The idea of this additional check was to ensure that the request cannot be fulfilled with the current buffer contents. Otherwise its not completely clear that async handler is really waiting for new data, as it should be. If you prefer, I can also remove this test again or set the timeout to 0 to avoid blocking. As the tests show, this part is already working fine. ;)
Yes, please. Both timeout 0 and removing the test is fine with me.
BTW, other patches look good to me.
Thanks, Jacek
Thanks, I've sent a new version.
participants (2)
-
Jacek Caban -
Sebastian Lackner