google / j2objc

A Java to iOS Objective-C translation tool and runtime.
http://j2objc.org
Apache License 2.0
5.99k stars 968 forks source link

IosHttpURLConnection should throw when interrupted #851

Open danieldickison opened 7 years ago

danieldickison commented 7 years ago

I'm using thread interruption to cancel ongoing http requests made with HttpURLConnection. This works correctly on Android by catching InterruptedIOException from methods like getResponseCode(). However, on iOS interruptions are ignored and instead getResponseCode() returns -1.

My proposal would be in the private getResponse() method instead of ignoring InterruptedException, create a new InterruptedIOException, assign it to responseException, and then raise it right there around here: https://github.com/google/j2objc/blob/fae801525820b8a7018e36e4d6c13c8b7ca152f3/jre_emul/Classes/com/google/j2objc/net/IosHttpURLConnection.java#L472

Also, I believe the synchronized-wait check needs to be a while loop to account for spurious wake-ups from Object.wait(): https://github.com/google/j2objc/blob/fae801525820b8a7018e36e4d6c13c8b7ca152f3/jre_emul/Classes/com/google/j2objc/net/IosHttpURLConnection.java#L461

I'm still troubleshooting my j2objc build issues so I haven't been able to try this out myself. Once I get it working, I can submit a pull request with these changes but I'm curious to hear if this seems like a problematic change to anybody.

lukhnos commented 7 years ago

My proposal would be in the private getResponse() method instead of ignoring InterruptedException, create a new InterruptedIOException, assign it to responseException, and then raise it right there around here: https://github.com/google/j2objc/blob/fae801525820b8a7018e36e4d6c13c8b7ca152f3/jre_emul/Classes/com/google/j2objc/net/IosHttpURLConnection.java#L472

You want to call cancelDataTask() before you raise the InterrputedIOException to cancel the underlying NSURLSession data task.

Since interrupting the request-making thread only cancels the request before it hears back from the server, how do you cancel a started request (say a long ongoing upload / download) on Android?

Also, I believe the synchronized-wait check needs to be a while loop to account for spurious wake-ups from Object.wait(): https://github.com/google/j2objc/blob/fae801525820b8a7018e36e4d6c13c8b7ca152f3/jre_emul/Classes/com/google/j2objc/net/IosHttpURLConnection.java#L461

What would be a scenario where that could happen?

danieldickison commented 7 years ago

Good point on calling cancelDataTask().

The reason for looping is that [Object.wait() is documented](https://developer.android.com/reference/java/lang/Object.html#wait()) to wake up spuriously on rare occasion:

A thread can also wake up without being notified, interrupted, or timing out, a so-called spurious wakeup. While this will rarely occur in practice, applications must guard against it by testing for the condition that should have caused the thread to be awakened, and continuing to wait if the condition is not satisfied. In other words, waits should always occur in loops…

lukhnos commented 7 years ago

Also, disconnect() already calls cancelDataTask() so I assume you can use that to cancel an ongoing request (after you've received the response code and header) on iOS at least, but do you use that on Android too? Or do you close the OutputStream / InputStream early for that purpose?

danieldickison commented 7 years ago

Actually we are calling disconnect() from our app code in a finally block before exiting due to an exception (InterruptedIOException in this case), but I agree that calling disconnect() from IosHttpURLConnection when handling the interruption makes sense.