Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cancel a job if timeout exceed #439

Closed
CristianCardosoA opened this issue Jul 17, 2018 · 6 comments
Closed

Cancel a job if timeout exceed #439

CristianCardosoA opened this issue Jul 17, 2018 · 6 comments

Comments

@CristianCardosoA
Copy link

CristianCardosoA commented Jul 17, 2018

I found a little problem, if the timeout is exceeded, the job is continue working until it finished.
The expected behavior that I supposed is that the job is going to be cancel if the time is exceeded.

waitForTheServerToSetUp() is a suspend function

This is the code

      val result = withTimeoutOrNull(mTimeoutDownloadDefault){

            job = launch(exceptionLogger) {

                      try {

                            download()

                      } catch (e : Exception) {

                             mError = true
                      }
            }

            job.join()
   }

   if(result == null){
      mError = true
   }


    private suspend fun download() {

    val job = launch {

        APIClient.getLifeEventTypes(this@DownloadManager).await()
    }
    job.join()
}
@qwwdfsad
Copy link
Collaborator

Hi!
I've checked, the timeouted job is cancelled properly.
Could you please provide a self-contained example which indicates the problem?
For me it seems that some part of APIClient.getLifeEventTypes is not cooperative

@CristianCardosoA
Copy link
Author

CristianCardosoA commented Jul 22, 2018

The problem I think as you said is not cooperative.

APIClient.getLifeEventTypes 

Is a suspendCancellableCoroutine, but the timeout doesn't cancel the parent job properly.

@qwwdfsad
Copy link
Collaborator

Then please provide a self-contained snippet of code which reproduces the problem, I'll take a look

@CristianCardosoA
Copy link
Author

CristianCardosoA commented Jul 22, 2018

Here is my code :D

fun main(){

  val result = withTimeoutOrNull(mTimeoutDownloadDefault){

          job = launch(exceptionLogger) {

                    try {

                          download()

                    } catch (e : Exception) {

                           mError = true
                    }
          }

          job.join()
   }

   If(result == null){
        mError = true
    }  
 }

private suspend fun download() {

    val job = launch {
        APIClient.getLifeEventTypes(this@DownloadManager).await()
    }
    job.join()
}



 fun getLifeEventTypes(context: Context) = async(CommonPool) {

    val clazz = Array<LifeEventType>::class.java

    val builtUri = Uri.parse(BuildConfig.BASE_URL)
            .buildUpon()
            .appendEncodedPath(context.getString(R.string.endpoint_url_lifeEvent_type))
            .build()

    try {

        process(context, builtUri.toString(), clazz, RequestJSONArrayRequestWithHeaders::class.java).filterNotNull().toTypedArray()

    } catch (e : Exception){

        return@async null
    }
}

private suspend inline fun <T,P> process(context: Context, url : String, clazz: Class<T>, typeOfRequest : Class<P>): T = suspendCancellableCoroutine { continuation ->


    val request: Request<*> = when(typeOfRequest){

        RequestStringRequestWithHeaders::class.java -> RequestStringRequestWithHeaders(context,url,continuation,clazz)
        RequestJSONArrayRequestWithHeaders::class.java -> RequestJSONArrayRequestWithHeaders(context,url,continuation,clazz)

        else -> {
            RequestStringRequestWithHeaders(context,url,continuation,clazz)
        }
    }

    request.retryPolicy = DefaultRetryPolicy(120_000,
            DefaultRetryPolicy.DEFAULT_MAX_RETRIES,
            DefaultRetryPolicy.DEFAULT_BACKOFF_MULT)

    NetworkManager.getInstance(context).addToRequestQueue(request)
    continuation.invokeOnCancellation {
        request.cancel()
    }
} 



 class RequestJSONArrayRequestWithHeaders<T> (val context : Context, url : String, continuation: Continuation<T>, clazz: Class<T>) : JsonArrayRequest(Method.GET, url, null,

        Response.Listener {

            val array : T = Gson().fromJson(it.toString(),clazz)
            Log.i(TAG, "$url: $it")
            continuation.resume(array)

        }, Response.ErrorListener {
            continuation.resumeWithException(it.cause ?: Exception("Unknown exception"))
        }
) {
    override fun getHeaders(): Map<String, String> {

        return FNAuthManager.authHeaders?.let {

            it[context.getString(R.string.endpoint_accept_key)] = context.getString(R.string.endpoint_json)
            Log.i(TAG, it.toString())
            it
        } ?: run<JsonArrayRequest, HashMap<String, String>> {
            HashMap()
        }
    }
}

@qwwdfsad
Copy link
Collaborator

qwwdfsad commented Jul 25, 2018

It looks like you forgot to instantiate a parent:

private suspend fun download() {

    val job = launch {
        APIClient.getLifeEventTypes(this@DownloadManager).await()
    }
    job.join()
}

When block responsible for downloading is cancelled, neither inner launch nor nested async are cancelled.

To properly cancel it, you should pass coroutineContext or parent explicitly:

private suspend fun download() {

    val job = launch(coroutineContext) {
        // getLifeEventTypes should start with async(CommonPool, parent = coroutineContext[Job])
        APIClient.getLifeEventTypes(this@DownloadManager).await()
    }
    job.join()
}

Hopefully it will be fixed automagically with #410

@CristianCardosoA
Copy link
Author

Thanks ! I will try that and I'll post my results based on your comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants