I'll upload another version with the modifications to SetState and the removal of the infinite loop in HandleStateMachine.
======================================================================== http://mondrian.corp.google.com/file/10462049///depot/googleclient/gears/opensource/gears/localserver/android/http_request_android.cc?a=1 File //depot/googleclient/gears/opensource/gears/localserver/android/http_request_android.cc (snapshot 1) ------------------------------------ Line 612: void HttpRequestAndroid::SetState(State state) { On 12:27 pm, andreip wrote: > Right now the functors change the state by setting instance_->state_ directly. > But we also have the SetState() method, so it's not clear when one should use > SetState and when one should just set state_ directly. > > How about making SetState() return a bool instead? Return true if the state was > changed successfully and false otherwise. In the functors we can then look at > the return value and decide whether to invoke HandleStateMachine() or not. In > SetState() we check which thread we are on and then check the conditions that > apply to each thread: was_aborted_ must be false for the state to be changed on > the main thread and !(was_aborted_ && (state != STATE_CHILD_ABORT)) for the > state to be changed on the child thread. I'd simply call SetState from the functors, and call HandleStateMachine() in SetState(), as needed. ------------------------------------ Line 643: // Note that every Child states can also transition to CHILD_ABORT On 12:30 pm, andreip wrote: > Child state Done. ------------------------------------ Line 644: // in case of error, and that CHILD_ABORT transition to MAIN_COMPLETE: On 12:30 pm, andreip wrote: > CHILD_ABORT can only transition to MAIN_COMPLETE. Done. ------------------------------------ Line 659: // call interrupt -- we are already past the normal request execution. On 12:36 pm, andreip wrote: > I don't think it matters if was_aborted_ is true or false. At that point there > are no blocking operations on the Java side. If there were, then the child > thread would be blocked so it could not enter the ABORT state and call > interrupt. Perhaps we want leave the call to interrupt() in the Abort() method, > as before? Done. ------------------------------------ Line 665: for (;;) { On 12:42 pm, andreip wrote: > I wonder if we can eliminate this loop altogether. If the child thread sends > functors to itself, the state machine will be automatically handled for us by > the thread's message loop. yes, that makes sense. ------------------------------------ Line 799: GetMethod(JAVA_METHOD_INTERRUPT)); On 12:37 pm, andreip wrote: > Move this to Abort()? Done. ------------------------------------ Line 826: if (IsChildThread()) { On 3:15 pm, andreip wrote: > MAIN_COMPLETE is now guaranteed to happen on the main thread only. Done. ------------------------------------ Line 1420: // We simply switch the child to the STATE_CHILD_ABORT state. On 12:38 pm, andreip wrote: > Note that this won't abort the child immediately. Rather it will wait until the > child finishes processing its current state (this can be receiving 1GB of data > so it'll take a while) and then it will proceed directly to the ABORT state and > terminate. Leave the interrupt call after SwitchToChildThreadState? ok ======================================================================== http://mondrian.corp.google.com/file/10462049///depot/googleclient/gears/opensource/gears/localserver/android/http_request_android.h?a=1 File //depot/googleclient/gears/opensource/gears/localserver/android/http_request_android.h (snapshot 1) ------------------------------------ Line 145: instance_->Ref(); On 12:33 pm, andreip wrote: > Say that we need to Ref() since we need to keep the instance alive while the > thread is running. Done. ------------------------------------ Line 150: instance_->Unref(); On 12:33 pm, andreip wrote: > assert(instance_->GetRef() == 1) ? no -- the rc count could be one or more than one here (if the instance wasn't gc by the javascript, or if we transition using a functor to MAIN_IDLE as you suggests) ------------------------------------ Line 226: // STATE_MAIN_COMPLETE. On 12:16 pm, andreip wrote: > This last sentence doesn't really make sense to me..? Done. ------------------------------------ Line 233: // invoked final processing. On 12:06 pm, andreip wrote: > This comment is out of date? The can never delete the instance. Assert that the > refcount is > 1 (i.e. instance->GetRef() > 1). if we transition via a functor, this could still be the case -- e.g. we got gc-ed, then we go to MAIN_COMPLETE, stop the child, but then send a functor to transition to MAIN_IDLE -- the functor will be the last one holding a reference to the instance. ======================================================================== -- To respond, reply to this email or visit http://mondrian.corp.google.com/10462049
