Meanwhile I've implemented my suggestions in 10520504, unit tests are green.
On Wed, Mar 18, 2009 at 4:11 PM, Nicolas Roard <[email protected]> wrote: > 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 >
