Re: [2/3] wininet: Start the first chunk as soon as we have some data.
Hi Hans, On 19.07.2016 11:27, Hans Leidekker wrote:
Signed-off-by: Hans Leidekker <hans(a)codeweavers.com> --- dlls/wininet/http.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/dlls/wininet/http.c b/dlls/wininet/http.c index 2c62d97..c4cf66b 100644 --- a/dlls/wininet/http.c +++ b/dlls/wininet/http.c @@ -2921,6 +2921,7 @@ static DWORD set_content_length(http_request_t *request) !strcmpiW(encoding, szChunked)) { chunked_stream_t *chunked_stream; + DWORD res;
chunked_stream = heap_alloc(sizeof(*chunked_stream)); if(!chunked_stream) @@ -2935,6 +2936,12 @@ static DWORD set_content_length(http_request_t *request) memcpy(chunked_stream->buf, request->read_buf+request->read_pos, request->read_size); chunked_stream->buf_size = request->read_size; request->read_size = request->read_pos = 0; + + res = start_next_chunk(chunked_stream, request); + if (res != ERROR_SUCCESS) { + heap_free(chunked_stream); + return res; + }
It seems to me that you're fixing a problem is a wrong place. start_next_chunk should be called by HTTP_ReceiveRequestData via refill_read_buffer anyway. I'd suggest to investigate why it doesn't work in your case. Also, see the attached test. We shouldn't need any chunk to complete requests. It's an existing problem in current Wine, which may be unrelated to your problem, but having such blocking calls here is not a step in the right direction. Thanks, Jacek
On Thu, 2016-07-21 at 14:25 +0200, Jacek Caban wrote:
--- dlls/wininet/http.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/dlls/wininet/http.c b/dlls/wininet/http.c index 2c62d97..c4cf66b 100644 --- a/dlls/wininet/http.c +++ b/dlls/wininet/http.c @@ -2921,6 +2921,7 @@ static DWORD set_content_length(http_request_t *request) !strcmpiW(encoding, szChunked)) { chunked_stream_t *chunked_stream; + DWORD res;
chunked_stream = heap_alloc(sizeof(*chunked_stream)); if(!chunked_stream) @@ -2935,6 +2936,12 @@ static DWORD set_content_length(http_request_t *request) memcpy(chunked_stream->buf, request->read_buf+request->read_pos, request->read_size); chunked_stream->buf_size = request->read_size; request->read_size = request->read_pos = 0; + + res = start_next_chunk(chunked_stream, request); + if (res != ERROR_SUCCESS) { + heap_free(chunked_stream); + return res; + }
It seems to me that you're fixing a problem is a wrong place. start_next_chunk should be called by HTTP_ReceiveRequestData via refill_read_buffer anyway. I'd suggest to investigate why it doesn't work in your case.
This is required to make get_avail_data return the correct number of bytes. Note that the line above sets the read size to 0, so (before my second patch) the next call to get_avail_data would return 0. This may trigger a read beyond available data. It is what happens with Outlook.
Also, see the attached test. We shouldn't need any chunk to complete requests. It's an existing problem in current Wine, which may be unrelated to your problem, but having such blocking calls here is not a step in the right direction.
Your test hangs with and without this patch. Why is having a blocking call here a problem? It would happen right after reading the headers, also blocking calls. Reading the header of the first chunk at this point may actually be the right thing to do. This way we can answer a query for available data without potentially blocking.
Hi Hans, On 21.07.2016 16:32, Hans Leidekker wrote:
On Thu, 2016-07-21 at 14:25 +0200, Jacek Caban wrote:
--- dlls/wininet/http.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/dlls/wininet/http.c b/dlls/wininet/http.c index 2c62d97..c4cf66b 100644 --- a/dlls/wininet/http.c +++ b/dlls/wininet/http.c @@ -2921,6 +2921,7 @@ static DWORD set_content_length(http_request_t *request) !strcmpiW(encoding, szChunked)) { chunked_stream_t *chunked_stream; + DWORD res;
chunked_stream = heap_alloc(sizeof(*chunked_stream)); if(!chunked_stream) @@ -2935,6 +2936,12 @@ static DWORD set_content_length(http_request_t *request) memcpy(chunked_stream->buf, request->read_buf+request->read_pos, request->read_size); chunked_stream->buf_size = request->read_size; request->read_size = request->read_pos = 0; + + res = start_next_chunk(chunked_stream, request); + if (res != ERROR_SUCCESS) { + heap_free(chunked_stream); + return res; + } It seems to me that you're fixing a problem is a wrong place. start_next_chunk should be called by HTTP_ReceiveRequestData via refill_read_buffer anyway. I'd suggest to investigate why it doesn't work in your case. This is required to make get_avail_data return the correct number of bytes. Note that the line above sets the read size to 0, so (before my second patch) the next call to get_avail_data would return 0. This may trigger a read beyond available data. It is what happens with Outlook.
Again, refill_read_buffer usually takes care of that. I think it probably didn't work in your case due to quirks we needed for non-blocking reads. I sent a patch that should improve chunked stream behaviour a lot [1]. There is still more work that could be done (like improvement of chunked_get_avail_data or taking advantage of now properly working non-blocking reads to improve InternetRead* functions), but I expect it to fix the problem that you were trying to solve with this patch. It's likely that your patch 3/3 may also not be needed (and if it is, it will need adjustment for the new way of storing chunk size).
Also, see the attached test. We shouldn't need any chunk to complete requests. It's an existing problem in current Wine, which may be unrelated to your problem, but having such blocking calls here is not a step in the right direction. Your test hangs with and without this patch. Why is having a blocking call here a problem?
Although it hangs on current Wine, it doesn't on Windows, and it shows that blocking reads should not be used here. Thanks, Jacek [1] http://source.winehq.org/patches/data/125006
participants (2)
-
Hans Leidekker -
Jacek Caban