[aerogear-dev] AGDROID-11
Summers Pittman
supittma at redhat.com
Thu Aug 15 14:11:22 EDT 2013
On 08/15/2013 02:08 PM, Yavuz Selim YILMAZ wrote:
> Hi,
>
> Some comments inline.
>
> And if that sounds good, I will issue a PR based on this direction,
> and more reviews/additions/changes would then be easier. wdyt?
Excellent plan.
>
> Regards.
>
> ---
> Yavuz Selim Yilmaz
> SUNY at Buffalo
> Computer Science and Engineering
> PhD Candidate
>
> On Aug 15, 2013, at 1:46 PM, Summers Pittman <supittma at redhat.com
> <mailto:supittma at redhat.com>> wrote:
>
>> On 08/15/2013 12:23 PM, Yavuz Selim YILMAZ wrote:
>>> So, if I get it right, each pipe will have it's runnable list. When
>>> the pipe is cancelled, it'll remove all waiting runnable's from
>>> threadpoolexecutor, and cancel the ones currently running. Am I right?
>> Not exactly. When the Runnable is finished, it removes itself form
>> the thread pool executor. You will need some other collection which
>> is modified when a pipe operation begins or completes.
>
> So, when I cancel currently running runnable's,
> they'll (automatically) be removed from the thread pool executor. But,
> there might be some awaiting runnable's in the thread pool executor
> too, or there can't be? I feel like, considering two consecutive pipe
> request, one will most likely be waiting in the thread pool executor
> while the other is running (or at least there might be such cases
> depending on the system resources etc.)
I suppose so. In that case RestRunner may need to have a check to see
if it has been canceled before it starts running.
>
>>>
>>> If so, then what'd be the proper way of cleaning the successfully
>>> completed runnable's from pipe's list?
>> For example RestAdapter.java#131:
>>
>> if (exception == null) {
>> runnableCollection.remove(this);
>> callback.onSuccess(this.result);
>> } else {
>> runnableCollection.remove(this);
>> callback.onFailure(exception);
>> }
>>
>> Would be my first guess.
>
> In this case, since the runnable will be a static nested class, it'll
> need a reference to the outer (i.e. RestAdapter) class. Am I right?
Yes.
>
>>
>>>
>>> For the loader part, I'll check on how to implement your suggestion,
>>>
>>> Regards,
>>>
>>> ---
>>> Yavuz Selim Yilmaz
>>> SUNY at Buffalo
>>> Computer Science and Engineering
>>> PhD Candidate
>>>
>>> On Aug 15, 2013, at 10:54 AM, Summers Pittman <supittma at redhat.com
>>> <mailto:supittma at redhat.com>> wrote:
>>>
>>>> Wow you don't make this easy on us ;)
>>>>
>>>> Killing the thread/thread pool is probably a bad idea. It may be
>>>> cleaner to make the anonymous runner classes in RestAdatper into
>>>> static classes which implement Runnable and provide a cancel
>>>> method. In the RestAdapter you could do some bookkeeping to
>>>> provide references to the Threads so you can call cancel on each
>>>> one which is currently running.
>>>>
>>>> Also, the LoaderAdapters will need a different solution (but theirs
>>>> is MUCH easier, you just call cancelLoader and implement onCancel
>>>> in the Loader implementations).
>>>>
>>>> Summers
>>>>
>>>> PS bookkeeping is one of my favorite words.
>>>>
>>>>
>>>> On 08/14/2013 03:36 PM, Yavuz Selim YILMAZ wrote:
>>>>> Hi all,
>>>>>
>>>>> I am working on AGDROID-11
>>>>> [https://issues.jboss.org/browse/AGDROID-11]. As suggested on the
>>>>> JIRA, I started with investigating how AeroGear iOS handles pipe
>>>>> cancel operation.
>>>>>
>>>>> -----------------------------------------------------------------------
>>>>> ---- Skip if you don't want to verify my iOS tracking ----
>>>>> -----------------------------------------------------------------------
>>>>> ------------------------------ Start ---------------------------------
>>>>> -----------------------------------------------------------------------
>>>>>
>>>>> First, here is what I could track on the iOS side (please correct
>>>>> me if there's any mistake):
>>>>>
>>>>> - AGRESTPipe cancel method simply calls the cancelAllOperations on
>>>>> its restClient's operationQueue [1].
>>>>> - AGRESTPipe's restClient is a AGHttpClient [2].
>>>>> - AGHttpClient is a AFHttpClient [3].
>>>>> - AFHttpClient's operationQueue is a NSOperationQueue [4].
>>>>> - Calling cancelAllOperations on NSOperationQueue sends cancel
>>>>> message to all Operations in the queue [5]. If the operation is
>>>>> still in the queue (waiting to be executed), they are not being
>>>>> executed. If the operation is alread started to it's execution,
>>>>> it's up to the "Operation" how to respond to cancel message.
>>>>> - Sending cancel message to operations sets their isCancelled
>>>>> flag. Any custom NSOperation extension is recommended to handle
>>>>> this cancel operation [6].
>>>>> - AGHttpClient creates the operation by calling
>>>>> HTTPRequestOperationWithRequest [7], which is AFHttpClient method.
>>>>> It creates either custom (AFImage, AFJSON, AFPropertyList, AFXML)
>>>>> operation [8] or AFHttpRequestOperation [9]. All custom operations
>>>>> are AFHttpRequestOperation, which is a AFURLConnectionOperation [10].
>>>>> - AFURLConnectionOperation implements cancel method [11] and
>>>>> simply cancels the operation's NSURLConnection, the connection is
>>>>> cancelled and no delegate is called [12].
>>>>> - Then AFURLConnectionOperation calls its delegate method manually
>>>>> to finish the operation [13].
>>>>> - The delegate handles the case [14] and finishes the operation [15].
>>>>>
>>>>> -----------------------------------------------------------------------
>>>>> ------------------------------- End ---------------------------------
>>>>> -----------------------------------------------------------------------
>>>>> ---- Skip if you don't want to verify my iOS tracking ----
>>>>> -----------------------------------------------------------------------
>>>>>
>>>>>
>>>>>
>>>>> Simply, what I understand is, operation itself is responsible to
>>>>> handle the cancel message sent by any pipe implementation. And on
>>>>> the Android side, the operations are runnables [16, 17, 18, 19] as
>>>>> far as I could interpret. So, these runnables should be able to
>>>>> handle cancel messages which means they should catch or throw the
>>>>> thrown InterruptedException.
>>>>>
>>>>> So, the problem is, find a way to send cancel message to the
>>>>> tasks, and handle the cancel message on the tasks.
>>>>>
>>>>>
>>>>> HOW TO SEND CANCEL MESSAGE TO THE TASKS?
>>>>>
>>>>> 1ST PROPOSAL:
>>>>>
>>>>> RestAdapter is using ThreadPoolExecutor [20]. So, one way to send
>>>>> cancel message to all running and queued runnables is to call
>>>>> shutDownNow on the ThreadPoolExecutor [21]. It has some
>>>>> limitations. First, shut down destroys the ThreadPoolExecutor, so
>>>>> it needs to be recreated. Second, the current ThredPoolExecutor is
>>>>> static, means it will cancel all pipes if it is shut down, so it
>>>>> needs to be an instance variable instead of static.
>>>>>
>>>>>
>>>>> 2ND PROPOSAL:
>>>>>
>>>>> Instead of calling THREAD_POOL_EXECUTOR.execute(), if we call
>>>>> THREAD_POOL_EXECUTOR.submit() [22], then it returns a Future
>>>>> representing the task. Future has cancel() [23], which prevents
>>>>> running the task if it is not started, and interrupts the task if
>>>>> it is currently running. Therefore, if we have a collection of
>>>>> <Future> in RestAdapter (i.e. pipe), then we can cancel the
>>>>> operations by iterating over the collection and calling cancel()
>>>>> on each Future. In this case, at some point, we need to clean
>>>>> <Future> collection to get rid of the already cancelled tasks.
>>>>>
>>>>>
>>>>>
>>>>> HOW TO HANDLE THE CANCEL MESSAGE ON THE TASKS?
>>>>>
>>>>> I could only come up with a single solution, which is, adding
>>>>> another catch block to each of the runnables [16, 17, 18, 19].
>>>>> Here is what it will look like:
>>>>>
>>>>> try {
>>>>> this.result = ...
>>>>> } catch (InterruptedException e) {
>>>>> return; // don't call the callbacks, may need to check the
>>>>> cause of the interrupt???
>>>>> } catch (Exception e) {
>>>>> Log.e(TAG, e.getMessage(), e);
>>>>> this.exception = e;
>>>>> }
>>>>>
>>>>>
>>>>>
>>>>> So, I would like to hear what you think about my solution, and if
>>>>> you have any corrections, better solutions, comments, anything...
>>>>> All are most welcome.
>>>>>
>>>>>
>>>>> Kind regards,
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> --- REFERENCES ---
>>>>> [1]
>>>>> https://github.com/aerogear/aerogear-ios/blob/master/AeroGear-iOS/AeroGear-iOS/pipeline/AGRESTPipe.m#L283
>>>>> [2]
>>>>> https://github.com/aerogear/aerogear-ios/blob/master/AeroGear-iOS/AeroGear-iOS/pipeline/AGRESTPipe.m#L71
>>>>> [3]
>>>>> https://github.com/aerogear/aerogear-ios/blob/master/AeroGear-iOS/AeroGear-iOS/core/AGHttpClient.h#L21
>>>>> [4]
>>>>> https://github.com/AFNetworking/AFNetworking/blob/master/AFNetworking/AFHTTPClient.m#L189
>>>>> [5]
>>>>> http://developer.apple.com/library/ios/DOCUMENTATION/Cocoa/Reference/NSOperationQueue_class/Reference/Reference.html#//apple_ref/occ/instm/NSOperationQueue/cancelAllOperations
>>>>> [6]
>>>>> http://developer.apple.com/library/mac/documentation/Cocoa/Reference/NSOperation_class/Reference/Reference.html#//apple_ref/doc/uid/TP40004591-RH2-SW18
>>>>> [7]
>>>>> https://github.com/aerogear/aerogear-ios/blob/master/AeroGear-iOS/AeroGear-iOS/core/AGHttpClient.m#L158
>>>>> [8]
>>>>> https://github.com/AFNetworking/AFNetworking/blob/master/AFNetworking/AFHTTPClient.m#L554
>>>>> [9]
>>>>> https://github.com/AFNetworking/AFNetworking/blob/master/AFNetworking/AFHTTPClient.m#L560
>>>>> [10]
>>>>> https://github.com/AFNetworking/AFNetworking/blob/master/AFNetworking/AFHTTPRequestOperation.h#L29
>>>>> [11]
>>>>> https://github.com/AFNetworking/AFNetworking/blob/master/AFNetworking/AFURLConnectionOperation.m#L548
>>>>> [12]
>>>>> http://developer.apple.com/library/mac/documentation/Cocoa/Reference/Foundation/Classes/NSURLConnection_Class/Reference/Reference.html#//apple_ref/occ/instm/NSURLConnection/cancel
>>>>> [13]
>>>>> https://github.com/AFNetworking/AFNetworking/blob/master/AFNetworking/AFURLConnectionOperation.m#L573
>>>>> [14]
>>>>> https://github.com/AFNetworking/AFNetworking/blob/master/AFNetworking/AFURLConnectionOperation.m#L760
>>>>> [15]
>>>>> https://github.com/AFNetworking/AFNetworking/blob/master/AFNetworking/AFURLConnectionOperation.m#L540
>>>>>
>>>>> [16]
>>>>> https://github.com/aerogear/aerogear-android/blob/master/src/org/jboss/aerogear/android/impl/pipeline/RestAdapter.java#L119
>>>>> [17]
>>>>> https://github.com/aerogear/aerogear-android/blob/master/src/org/jboss/aerogear/android/impl/pipeline/RestAdapter.java#L146
>>>>> [18]
>>>>> https://github.com/aerogear/aerogear-android/blob/master/src/org/jboss/aerogear/android/impl/pipeline/RestAdapter.java#L170
>>>>> [19]
>>>>> https://github.com/aerogear/aerogear-android/blob/master/src/org/jboss/aerogear/android/impl/pipeline/RestAdapter.java#L198
>>>>> [20]
>>>>> https://github.com/aerogear/aerogear-android/blob/master/src/org/jboss/aerogear/android/impl/pipeline/RestAdapter.java#L50
>>>>> [21]
>>>>> http://developer.android.com/reference/java/util/concurrent/ThreadPoolExecutor.html#shutdownNow()
>>>>> <http://developer.android.com/reference/java/util/concurrent/ThreadPoolExecutor.html#shutdownNow%28%29>
>>>>> [22]
>>>>> http://developer.android.com/reference/java/util/concurrent/AbstractExecutorService.html#submit(java.lang.Runnable)
>>>>> <http://developer.android.com/reference/java/util/concurrent/AbstractExecutorService.html#submit%28java.lang.Runnable%29>
>>>>> [23]
>>>>> http://developer.android.com/reference/java/util/concurrent/Future.html#cancel(boolean)
>>>>> <http://developer.android.com/reference/java/util/concurrent/Future.html#cancel%28boolean%29>
>>>>>
>>>>> ---
>>>>> Yavuz Selim Yilmaz
>>>>> SUNY at Buffalo
>>>>> Computer Science and Engineering
>>>>> PhD Candidate
>>>>>
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> aerogear-dev mailing list
>>>>> aerogear-dev at lists.jboss.org
>>>>> https://lists.jboss.org/mailman/listinfo/aerogear-dev
>>>>
>>>> _______________________________________________
>>>> aerogear-dev mailing list
>>>> aerogear-dev at lists.jboss.org <mailto:aerogear-dev at lists.jboss.org>
>>>> https://lists.jboss.org/mailman/listinfo/aerogear-dev
>>>
>>>
>>>
>>> _______________________________________________
>>> aerogear-dev mailing list
>>> aerogear-dev at lists.jboss.org
>>> https://lists.jboss.org/mailman/listinfo/aerogear-dev
>>
>> _______________________________________________
>> aerogear-dev mailing list
>> aerogear-dev at lists.jboss.org <mailto:aerogear-dev at lists.jboss.org>
>> https://lists.jboss.org/mailman/listinfo/aerogear-dev
>
>
>
> _______________________________________________
> aerogear-dev mailing list
> aerogear-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/aerogear-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.jboss.org/pipermail/aerogear-dev/attachments/20130815/7b0eb263/attachment-0001.html
More information about the aerogear-dev
mailing list