Hello nicolasroard, steveblock,
I'd like you to do a code review. Please execute
g4 diff -c 9187927
or point your web browser to
http://mondrian/9187927
to review the following code:
Change 9187927 by [EMAIL PROTECTED] on 2008/11/27 18:23:22 *pending*
Fix deadlock in HttpRequestAndroid::Abort().
The cause of the deadlock was caused by a misunderstanding on
how the Gears message queues work:
- Abort() is a Gears API that can be called as a result of a
message being handled.
- Abort() was running the message loop recursively inside a
while loop whose the exit condition depended on another
message being processed.
The above designed caused a deadlock when the message that was
meant to cause the while loop to exit was received before the
Abort message. The first message was pumped out of the queue
after the Abort message and placed into a local queue, waiting
for its processing to happen after the processing of the Abort
message completed. However the processing of the Abort message
cannot ever complete since its while loop depends on the first
message being processed. Running the thread message queue
inside the while loop did not help since the desired message
had, alas, been pumped out already and the thread message
queue is empty.
PRESUBMIT=passed
R=nicolasroard,steveblock
[EMAIL PROTECTED]
DELTA=113 (39 added, 60 deleted, 14 changed)
OCL=9187927
Affected files ...
... //depot/googleclient/gears/opensource/gears/base/common/message_queue.h#16
edit
...
//depot/googleclient/gears/opensource/gears/base/common/message_queue_android.cc#8
edit
...
//depot/googleclient/gears/opensource/gears/localserver/android/http_request_android.cc#2
edit
...
//depot/googleclient/gears/opensource/gears/localserver/android/http_request_android.h#1
edit
113 delta lines: 39 added, 60 deleted, 14 changed
Also consider running:
g4 lint -c 9187927
which verifies that the changelist doesn't introduce new style violations.
If you can't do the review, please let me know as soon as possible. During
your review, please ensure that all new code has corresponding unit tests and
that existing unit tests are updated appropriately. Visit
http://www/eng/code_review.html for more information.
This is a semiautomated message from "g4 mail". Complaints or suggestions?
Mail [EMAIL PROTECTED]
Change 9187927 by [EMAIL PROTECTED] on 2008/11/27 18:23:22 *pending*
Fix deadlock in HttpRequestAndroid::Abort()
Affected files ...
... //depot/googleclient/gears/opensource/gears/base/common/message_queue.h#16
edit
...
//depot/googleclient/gears/opensource/gears/base/common/message_queue_android.cc#8
edit
...
//depot/googleclient/gears/opensource/gears/localserver/android/http_request_android.cc#2
edit
...
//depot/googleclient/gears/opensource/gears/localserver/android/http_request_android.h#1
edit
==== //depot/googleclient/gears/opensource/gears/base/common/message_queue.h#16
-
/home/andreip/Gears/googleclient/gears/opensource/gears/base/common/message_queue.h
====
# action=edit type=text
--- googleclient/gears/opensource/gears/base/common/message_queue.h
2008-11-27 18:28:47.000000000 +0000
+++ googleclient/gears/opensource/gears/base/common/message_queue.h
2008-11-27 17:59:09.000000000 +0000
@@ -130,10 +130,6 @@
// Possible fix: Add Event::Clear() and call it
// from AsyncCallback.
static void RunOnce();
- // As above, but wait at most a given number of
- // milliseconds. Returns true if the queue was processed, or false
- // if timed out.
- static bool RunOnceWithTimeout(int milliseconds);
// Stops the message loop for the given thread. The target thread
// must be looping in Start().
static void Stop(ThreadId thread_id);
====
//depot/googleclient/gears/opensource/gears/base/common/message_queue_android.cc#8
-
/home/andreip/Gears/googleclient/gears/opensource/gears/base/common/message_queue_android.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/base/common/message_queue_android.cc
2008-11-27 18:28:47.000000000 +0000
+++ googleclient/gears/opensource/gears/base/common/message_queue_android.cc
2008-11-27 18:00:27.000000000 +0000
@@ -88,10 +88,6 @@
// Waits for one or messages to arrive and dispatches them to the
// appropriate handlers.
void RunOnce();
- // Process the message queue, waiting at most for the given number
- // of milliseconds. Returns true if the queue was processed, or
- // false if timed out.
- bool RunOnceWithTimeout(int milliseconds);
// Dispatches them to the appropriate handlers.
void DispatchOnce();
@@ -253,15 +249,6 @@
ThreadId thread_id =
AndroidThreadMessageQueue::GetInstance()->GetCurrentThreadId();
AndroidThreadMessageQueue::GetQueue(thread_id)->RunOnce();
-}
-
-// static
-bool AndroidMessageLoop::RunOnceWithTimeout(int milliseconds) {
- // Run the loop once on the current thread.
- ThreadId thread_id =
- AndroidThreadMessageQueue::GetInstance()->GetCurrentThreadId();
- ThreadSpecificQueue *queue = AndroidThreadMessageQueue::GetQueue(thread_id);
- return queue->RunOnceWithTimeout(milliseconds);
}
// static
@@ -345,18 +332,6 @@
void ThreadSpecificQueue::RunOnce() {
event_.Wait();
DispatchOnce();
-}
-
-bool ThreadSpecificQueue::RunOnceWithTimeout(int milliseconds) {
- // Wait at most the given number of milliseconds.
- if (event_.WaitWithTimeout(milliseconds)) {
- // Signalled, dispatch messages.
- DispatchOnce();
- return true;
- } else {
- // Timed out.
- return false;
- }
}
void ThreadSpecificQueue::DispatchOnce() {
====
//depot/googleclient/gears/opensource/gears/localserver/android/http_request_android.cc#2
-
/home/andreip/Gears/googleclient/gears/opensource/gears/localserver/android/http_request_android.cc
====
# action=edit type=text
---
googleclient/gears/opensource/gears/localserver/android/http_request_android.cc
2008-11-27 18:28:47.000000000 +0000
+++
googleclient/gears/opensource/gears/localserver/android/http_request_android.cc
2008-11-27 18:27:29.000000000 +0000
@@ -159,8 +159,13 @@
// from a backgroudn thread. SafeHttpRequest calls Create() to
// construct a raw HttpRequestAndroid as required.
bool HttpRequest::CreateSafeRequest(scoped_refptr<HttpRequest> *request) {
- request->reset(new SafeHttpRequest(HttpRequestAndroid::main_thread_id_));
- return true;
+ if (HttpRequestAndroid::IsMainThread()) {
+ request->reset(new HttpRequestAndroid());
+ return true;
+ } else {
+ request->reset(new SafeHttpRequest(HttpRequestAndroid::main_thread_id_));
+ return true;
+ }
}
@@ -548,6 +553,8 @@
state));
}
+// The function below is only called by the state machine so the call
+// to SetState is valid.
void HttpRequestAndroid::SwitchToChildThreadState(State state) {
assert(IsMainThread());
LOG(("Transferring to child thread with state %s\n", GetStateName(state)));
@@ -569,6 +576,30 @@
GetStateName(state_)));
// Loop the state machine until the current thread is no longer in
// control.
+ // Note that state transitions happen only in the following circumstances
+ // that denote the 'normal operation' of an HTTP request:
+ // MAIN_IDLE->MAIN_REQUEST in HttpRequestAndroid::Send()
+ // MAIN_REQUEST->MAIN_PARSE_HEADERS directly in HandleStateMachine()
+ // MAIN_PARSE_HEADERS->CHILD_CONNECT_TO_REMOTE via async functor
+ // CHILD_CONNECT_TO_REMOTE->MAIN_CONNECTED in via async functor
+ // MAIN_CONNECTED->CHILD_POST via async functor
+ // CHILD_POST->MAIN_PARSE_HEADERS via async functor
+ // MAIN_CONNECTED->MAIN_PARSE_HEADERS directly in HandleStateMachine()
+ // MAIN_PARSE_HEADERS->CHILD_RECEIVE via async functor
+ // MAIN_PARSE_HEADERS->MAIN_REQUEST directly in HandleStateMachine()
+ // CHILD_RECEIVE->MAIN_RECEIVED via async functor
+ // CHILD_RECEIVE->MAIN_COMPLETE via async functor
+ // MAIN_RECEIVED->MAIN_COMPLETE directly in HandleStateMachine()
+
+ // Aside from the 'normal operation', we can receive at any point
+ // an 'Abort()'. If that happens, we need to guarantee that
+ // 1. If the request is already complete, Abort() noops.
+ // 2. The background thread exits before Abort() returns.
+ // 3. The state is reset to MAIN_COMPLETE and then MAIN_IDLE
+ // 4. Any queued functors that are processed after abort need
+ // need to be ignored since they would cause an invalid transition
+ // from MAIN_IDLE to some random state.
+
for (;;) {
switch (state_) {
case STATE_MAIN_IDLE:
@@ -608,10 +639,6 @@
case STATE_MAIN_CONNECTED:
assert(IsMainThread());
- if (was_aborted_) {
- SetState(STATE_MAIN_COMPLETE);
- break;
- }
// Increment ready state visible by observers to SENT.
SetReadyState(SENT);
// Child thread successfully connected to the remote site. If
@@ -637,10 +664,6 @@
case STATE_MAIN_PARSE_HEADERS:
assert(IsMainThread());
- if (was_aborted_) {
- SetState(STATE_MAIN_COMPLETE);
- break;
- }
if (ParseHeaders()) {
// Test for redirect.
std::string16 redirect_url = GetRedirect();
@@ -700,10 +723,6 @@
case STATE_MAIN_RECEIVED: {
assert(IsMainThread());
- if (was_aborted_) {
- SetState(STATE_MAIN_COMPLETE);
- break;
- }
// Done, insert into cache if policy allows.
if (served_locally_) {
// Prevent re-saving something that either came out of cache
@@ -1281,37 +1300,18 @@
}
bool HttpRequestAndroid::Abort() {
- LOG(("Abort\n"));
- assert(IsMainThread());
-
- // Signal the child thread that it should stop any long term
- // blocking operations, e.g receiving or sending bulk data.
+ LOG(("Aborting\n"));
+ assert(IsMainThread());
was_aborted_ = true;
- // Wait for the state machine to go idle. Unfortunately, the
- // java.net.* operations are not easy to atomically interrupt, so we
- // need to resort to an ugly back-off-retry loop.
- int sleep_period_ms = 10;
- // state_ is only changed by the main thread, which gives us a good
- // rendezvous point to know that the child has handed us back
- // control.
- while (state_ != STATE_MAIN_IDLE) {
- LOG(("Waiting for idle\n"));
- // Interrupt the Java-side in case it's stuck in one of the
- // java.net.* methods.
+ if (state_ != STATE_MAIN_IDLE) {
+ // Signal the child thread that it should stop any long term
+ // blocking operations, e.g receiving or sending bulk data.
LOG(("Calling interrupt()\n"));
JniGetEnv()->CallVoidMethod(java_object_.Get(),
GetMethod(JAVA_METHOD_INTERRUPT));
- // Process the message queue.
- while (AndroidMessageLoop::RunOnceWithTimeout(sleep_period_ms)) {
- // No-op. Loop until out of messages and timed out, then resort
- // to another interrupt.
- }
- // Double the sleep period up to a maximum of 1.28 seconds.
- if (sleep_period_ms < 1280) {
- sleep_period_ms *= 2;
- }
- }
- SetReadyState(COMPLETE);
+ SetState(STATE_MAIN_COMPLETE);
+ HandleStateMachine();
+ }
LOG(("Abort complete\n"));
return true;
}
====
//depot/googleclient/gears/opensource/gears/localserver/android/http_request_android.h#1
-
/home/andreip/Gears/googleclient/gears/opensource/gears/localserver/android/http_request_android.h
====
# action=edit type=text
---
googleclient/gears/opensource/gears/localserver/android/http_request_android.h
2008-11-27 18:28:47.000000000 +0000
+++
googleclient/gears/opensource/gears/localserver/android/http_request_android.h
2008-11-27 18:21:03.000000000 +0000
@@ -137,7 +137,7 @@
public:
HttpThread(HttpRequestAndroid *instance) : instance_(instance) { }
protected:
- virtual void Run() { instance_->ThreadRun(); }
+ virtual void Run() { instance_->ThreadRun(); LOG(("Child exiting."));}
private:
HttpRequestAndroid *instance_;
};
@@ -192,14 +192,22 @@
virtual void Run() {
switch (target_) {
case TARGET_THREAD_MAIN:
- // Set the state now that we're in the main thread.
- instance_->state_ = new_state_;
- instance_->HandleStateMachine();
+ // If we are already IDLE, any state transition set via a
+ // functor comes too late so we ignore it.
+ if (instance_->state_ != STATE_MAIN_IDLE) {
+ // Set the state now that we're in the main thread.
+ instance_->state_ = new_state_;
+ LOG(("Processing state %d on the main thread", new_state_));
+ instance_->HandleStateMachine();
+ } else {
+ assert(instance_->was_aborted_ == true);
+ }
// Now potentially delete the instance, if this functor
// invoked final processing.
instance_->Unref();
break;
case TARGET_THREAD_CHILD:
+ LOG(("Processing state %d on the child thread", new_state_));
// The main thread should have set the state before
// switching to the child thread's control.
assert(instance_->state_ == new_state_);
