Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(5)

Issue 11361119: Keep track of the request and response state in the HTTP client connection (Closed)

Created:
8 years, 1 month ago by Søren Gjesse
Modified:
8 years, 1 month ago
Reviewers:
Mads Ager (google)
CC:
reviews_dartlang.org
Visibility:
Public.

Description

Keep track of the request and response state in the HTTP client connection In a HTTP client connection it is possible for the response to end before the request has been fully sent. This situation was not handled correctly. Now the HTTP client connection keeps track of the state of both request and response and doen not end the client connection before both are done. R=ager@google.com BUG=dart:6521 Committed: https://code.google.com/p/dart/source/detail?r=14559

Patch Set 1 #

Total comments: 6

Patch Set 2 : Addressed review comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+121 lines, -25 lines) Patch
M pkg/pkg.status View 1 chunk +0 lines, -2 lines 0 comments Download
M sdk/lib/io/http_impl.dart View 1 7 chunks +80 lines, -21 lines 0 comments Download
A tests/standalone/io/regress-6521.dart View 1 chunk +41 lines, -0 lines 0 comments Download
M tests/standalone/standalone.status View 1 chunk +0 lines, -2 lines 0 comments Download

Messages

Total messages: 3 (0 generated)
Søren Gjesse
8 years, 1 month ago (2012-11-06 09:05:53 UTC) #1
Mads Ager (google)
LGTM https://codereview.chromium.org/11361119/diff/1/sdk/lib/io/http_impl.dart File sdk/lib/io/http_impl.dart (right): https://codereview.chromium.org/11361119/diff/1/sdk/lib/io/http_impl.dart#newcode1980 sdk/lib/io/http_impl.dart:1980: bool get _isRequestDone => (_state & REQUEST_DONE) == ...
8 years, 1 month ago (2012-11-06 09:30:57 UTC) #2
Søren Gjesse
8 years, 1 month ago (2012-11-06 11:28:46 UTC) #3
https://codereview.chromium.org/11361119/diff/1/sdk/lib/io/http_impl.dart
File sdk/lib/io/http_impl.dart (right):

https://codereview.chromium.org/11361119/diff/1/sdk/lib/io/http_impl.dart#new...
sdk/lib/io/http_impl.dart:1980: bool get _isRequestDone => (_state &
REQUEST_DONE) == REQUEST_DONE;
On 2012/11/06 09:30:57, Mads Ager wrote:
> Maybe remove 'is' from all of these?
> 
> if (requestDone) ...;

I would have liked this as well. However there is already a method called
_requestDone.

https://codereview.chromium.org/11361119/diff/1/sdk/lib/io/http_impl.dart#new...
sdk/lib/io/http_impl.dart:1996: _pendingRetry = null;
On 2012/11/06 09:30:57, Mads Ager wrote:
> Should we move the resetting of these to null into the branches above? Or
maybe
> add an assert that we never have both _pendingRedirect and _pendingRetry?

Done (both).

https://codereview.chromium.org/11361119/diff/1/sdk/lib/io/http_impl.dart#new...
sdk/lib/io/http_impl.dart:2102: if (_isAllDone) {
On 2012/11/06 09:30:57, Mads Ager wrote:
> Maybe add a comment to retry and redirect that we postpone the actual
> retry/redirect until both request and response are done.

Done.

Powered by Google App Engine
This is Rietveld 408576698