21 Jul
2016
21 Jul
'16
12:25 p.m.
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
21 Jul
21 Jul
2:32 p.m.
New subject: [2/3] wininet: Start the first chunk as soon as we have some data.
On Thu, 2016-07-21 at 14:25 +0200, Jacek Caban wrote:
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.
---
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.
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.
1 Aug
1 Aug
2:41 p.m.
New subject: [2/3] wininet: Start the first chunk as soon as we have some data.
Hi Hans,
On 21.07.2016 16:32, Hans Leidekker wrote:
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
On Thu, 2016-07-21 at 14:25 +0200, Jacek Caban wrote:
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).
---
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.
Your test hangs with and without this patch. Why is
having a blocking
call here a problem?
2831
days inactive
2842
days old
2 comments
2 participants
participants (2)
-
Hans Leidekker -
Jacek Caban