From b5629da794cb3c1ca1970d206343743b165b9644 Mon Sep 17 00:00:00 2001 From: Steve Howard Date: Fri, 16 Jul 2010 14:28:35 -0700 Subject: Major refactoring of DownloadThread.run(). Motivation: I need to fix the handling of 302s, so that after a disconnect, subsequent retries will use the original URI, not the redirected one. Rather than store extra information in the DB, I'd like to just keep the redirected URI in memory and make the redirected request within the same DownloadThread. This involves working with the large-scale structure of DownloadThread.run(). Since run() was a ~700 line method, I didn't feel comfortable making such changes. So this change refactors run() into a ~80 line method which calls into a collection of ~20 other short methods. The state previously kept in local variables has been pulled into a couple of state-only inner classes. The error-handling control flow, formerly handled by "break http_request_loop" statements, is now handled by throwing a "StopRequest" exception. The remaining structure of run() has been simplified -- the outermost for loop, for example, could never actually repeat and has been removed for now. Some other bits of code have been cleaned up a bit, but the functionality has not been modified. There are many good next steps to this refactoring. Besides various other cleanup bits, a major improvement would be to consolidate the State/InnerState classes, move some functionality to this new class (there are many functions of the form "void foo(State)" which would be good candidates), and promote it to a top-level class. But I want to take things one step at a time, and I think what I've got here is a major improvement and should be enough to allow me to safely implement the changes to redirection handling. In the process of doing this refactoring I added many test cases to PublicApiFunctionalTest to exercise some of the pieces of code I was moving around. I also moved some test cases from DownloadManagerFunctionalTest. Over time I'd like to move everything over to use the PublicApiFunctionalTest approach, and then I may break that into some smaller suites. Other minor changes: * use longs instead of ints to track file sizes, as these may be getting quite large in the future * provide a default DB value of -1 for COLUMN_TOTAL_BYTES, as this simplifies some logic in DownloadThread * small extensions to MockResponse to faciliate new test cases Change-Id: If7862349296ad79ff6cdc97e554ad14c01ce1f49 --- .../AbstractDownloadManagerFunctionalTest.java | 12 +- .../downloads/DownloadManagerFunctionalTest.java | 42 ------ .../downloads/PublicApiFunctionalTest.java | 148 ++++++++++++++++++--- 3 files changed, 136 insertions(+), 66 deletions(-) (limited to 'tests/src/com/android/providers') diff --git a/tests/src/com/android/providers/downloads/AbstractDownloadManagerFunctionalTest.java b/tests/src/com/android/providers/downloads/AbstractDownloadManagerFunctionalTest.java index 326d9fff..92678fe3 100644 --- a/tests/src/com/android/providers/downloads/AbstractDownloadManagerFunctionalTest.java +++ b/tests/src/com/android/providers/downloads/AbstractDownloadManagerFunctionalTest.java @@ -195,10 +195,16 @@ public abstract class AbstractDownloadManagerFunctionalTest extends * Enqueue a response from the MockWebServer. */ MockResponse enqueueResponse(int status, String body) { + return enqueueResponse(status, body, true); + } + + MockResponse enqueueResponse(int status, String body, boolean includeContentType) { MockResponse response = new MockResponse() - .setResponseCode(status) - .setBody(body) - .addHeader("Content-type", "text/plain"); + .setResponseCode(status) + .setBody(body); + if (includeContentType) { + response.addHeader("Content-type", "text/plain"); + } mServer.enqueue(response); return response; } diff --git a/tests/src/com/android/providers/downloads/DownloadManagerFunctionalTest.java b/tests/src/com/android/providers/downloads/DownloadManagerFunctionalTest.java index 3cd9cf58..822ab54d 100644 --- a/tests/src/com/android/providers/downloads/DownloadManagerFunctionalTest.java +++ b/tests/src/com/android/providers/downloads/DownloadManagerFunctionalTest.java @@ -85,18 +85,6 @@ public class DownloadManagerFunctionalTest extends AbstractDownloadManagerFuncti runUntilStatus(downloadUri, Downloads.STATUS_SUCCESS); } - public void testRedirect() throws Exception { - enqueueEmptyResponse(301).addHeader("Location", mServer.getUrl("/other_path").toString()); - enqueueResponse(HTTP_OK, FILE_CONTENT); - Uri downloadUri = requestDownload("/path"); - RecordedRequest request = runUntilStatus(downloadUri, Downloads.STATUS_RUNNING_PAUSED); - assertEquals("/path", request.getPath()); - - mSystemFacade.incrementTimeMillis(RETRY_DELAY_MILLIS); - request = runUntilStatus(downloadUri, Downloads.STATUS_SUCCESS); - assertEquals("/other_path", request.getPath()); - } - public void testBasicConnectivityChanges() throws Exception { enqueueResponse(HTTP_OK, FILE_CONTENT); Uri downloadUri = requestDownload("/path"); @@ -134,36 +122,6 @@ public class DownloadManagerFunctionalTest extends AbstractDownloadManagerFuncti runUntilStatus(downloadUri, Downloads.STATUS_SUCCESS); } - public void testInterruptedDownload() throws Exception { - int initialLength = 5; - String etag = "my_etag"; - int totalLength = FILE_CONTENT.length(); - // the first response has normal headers but unexpectedly closes after initialLength bytes - enqueueResponse(HTTP_OK, FILE_CONTENT.substring(0, initialLength)) - .addHeader("Content-length", totalLength) - .addHeader("Etag", etag) - .setCloseConnectionAfter(true); - Uri downloadUri = requestDownload("/path"); - - runUntilStatus(downloadUri, Downloads.STATUS_RUNNING_PAUSED); - - mSystemFacade.incrementTimeMillis(RETRY_DELAY_MILLIS); - // the second response returns partial content for the rest of the data - enqueueResponse(HTTP_PARTIAL_CONTENT, FILE_CONTENT.substring(initialLength)) - .addHeader("Content-range", - "bytes " + initialLength + "-" + totalLength + "/" + totalLength) - .addHeader("Etag", etag); - // TODO: ideally we wouldn't need to call startService again, but there's a bug where the - // service won't retry a download until an intent comes in - RecordedRequest request = runUntilStatus(downloadUri, Downloads.STATUS_SUCCESS); - - List headers = request.getHeaders(); - assertTrue("No Range header: " + headers, - headers.contains("Range: bytes=" + initialLength + "-")); - assertTrue("No ETag header: " + headers, headers.contains("If-Match: " + etag)); - assertEquals(FILE_CONTENT, getDownloadContents(downloadUri)); - } - /** * Read a downloaded file from disk. */ diff --git a/tests/src/com/android/providers/downloads/PublicApiFunctionalTest.java b/tests/src/com/android/providers/downloads/PublicApiFunctionalTest.java index e34c66e6..b1ccc7ae 100644 --- a/tests/src/com/android/providers/downloads/PublicApiFunctionalTest.java +++ b/tests/src/com/android/providers/downloads/PublicApiFunctionalTest.java @@ -21,17 +21,24 @@ import android.net.ConnectivityManager; import android.net.DownloadManager; import android.net.Uri; import android.os.Environment; +import android.os.ParcelFileDescriptor; import android.test.suitebuilder.annotation.LargeTest; +import tests.http.MockResponse; import tests.http.RecordedRequest; import java.io.File; import java.io.FileInputStream; import java.io.InputStream; import java.net.MalformedURLException; +import java.util.List; @LargeTest public class PublicApiFunctionalTest extends AbstractDownloadManagerFunctionalTest { + private static final int HTTP_NOT_ACCEPTABLE = 406; + private static final int HTTP_LENGTH_REQUIRED = 411; private static final String REQUEST_PATH = "/path"; + private static final String REDIRECTED_PATH = "/other_path"; + private static final String ETAG = "my_etag"; class Download implements StatusReader { final long mId; @@ -73,8 +80,10 @@ public class PublicApiFunctionalTest extends AbstractDownloadManagerFunctionalTe } String getContents() throws Exception { - InputStream stream = new FileInputStream( - mManager.openDownloadedFile(mId).getFileDescriptor()); + ParcelFileDescriptor downloadedFile = mManager.openDownloadedFile(mId); + assertTrue("Invalid file descriptor: " + downloadedFile, + downloadedFile.getFileDescriptor().valid()); + InputStream stream = new FileInputStream(downloadedFile.getFileDescriptor()); try { return readStream(stream); } finally { @@ -161,43 +170,53 @@ public class PublicApiFunctionalTest extends AbstractDownloadManagerFunctionalTe public void testDownloadError() throws Exception { enqueueEmptyResponse(HTTP_NOT_FOUND); - Download download = enqueueRequest(getRequest()); - download.runUntilStatus(DownloadManager.STATUS_FAILED); - assertEquals(HTTP_NOT_FOUND, download.getLongField(DownloadManager.COLUMN_ERROR_CODE)); + runSimpleFailureTest(HTTP_NOT_FOUND); } public void testUnhandledHttpStatus() throws Exception { enqueueEmptyResponse(1234); // some invalid HTTP status - Download download = enqueueRequest(getRequest()); - download.runUntilStatus(DownloadManager.STATUS_FAILED); - assertEquals(DownloadManager.ERROR_UNHANDLED_HTTP_CODE, - download.getLongField(DownloadManager.COLUMN_ERROR_CODE)); + runSimpleFailureTest(DownloadManager.ERROR_UNHANDLED_HTTP_CODE); } public void testInterruptedDownload() throws Exception { int initialLength = 5; - String etag = "my_etag"; - int totalLength = FILE_CONTENT.length(); - // the first response has normal headers but unexpectedly closes after initialLength bytes - enqueueResponse(HTTP_OK, FILE_CONTENT.substring(0, initialLength)) - .addHeader("Content-length", totalLength) - .addHeader("Etag", etag) - .setCloseConnectionAfter(true); - Download download = enqueueRequest(getRequest()); + enqueueInterruptedDownloadResponses(initialLength); + Download download = enqueueRequest(getRequest()); download.runUntilStatus(DownloadManager.STATUS_PAUSED); assertEquals(initialLength, download.getLongField(DownloadManager.COLUMN_BYTES_DOWNLOADED_SO_FAR)); + assertEquals(FILE_CONTENT.length(), + download.getLongField(DownloadManager.COLUMN_TOTAL_SIZE_BYTES)); mSystemFacade.incrementTimeMillis(RETRY_DELAY_MILLIS); + RecordedRequest request = download.runUntilStatus(DownloadManager.STATUS_SUCCESSFUL); + assertEquals(FILE_CONTENT.length(), + download.getLongField(DownloadManager.COLUMN_BYTES_DOWNLOADED_SO_FAR)); + assertEquals(FILE_CONTENT, download.getContents()); + + List headers = request.getHeaders(); + assertTrue("No Range header: " + headers, + headers.contains("Range: bytes=" + initialLength + "-")); + assertTrue("No ETag header: " + headers, headers.contains("If-Match: " + ETAG)); + } + + private void enqueueInterruptedDownloadResponses(int initialLength) { + int totalLength = FILE_CONTENT.length(); + // the first response has normal headers but unexpectedly closes after initialLength bytes + enqueuePartialResponse(initialLength); // the second response returns partial content for the rest of the data enqueueResponse(HTTP_PARTIAL_CONTENT, FILE_CONTENT.substring(initialLength)) .addHeader("Content-range", "bytes " + initialLength + "-" + totalLength + "/" + totalLength) - .addHeader("Etag", etag); - download.runUntilStatus(DownloadManager.STATUS_SUCCESSFUL); - assertEquals(totalLength, - download.getLongField(DownloadManager.COLUMN_BYTES_DOWNLOADED_SO_FAR)); + .addHeader("Etag", ETAG); + } + + private MockResponse enqueuePartialResponse(int initialLength) { + return enqueueResponse(HTTP_OK, FILE_CONTENT.substring(0, initialLength)) + .addHeader("Content-length", FILE_CONTENT.length()) + .addHeader("Etag", ETAG) + .setCloseConnectionAfter(true); } public void testFiltering() throws Exception { @@ -323,6 +342,93 @@ public class PublicApiFunctionalTest extends AbstractDownloadManagerFunctionalTe } } + public void testRedirect301() throws Exception { + RecordedRequest lastRequest = runRedirectionTest(301); + // for 301, upon retry, we reuse the redirected URI + assertEquals(REDIRECTED_PATH, lastRequest.getPath()); + } + + // TODO: currently fails + public void disabledTestRedirect302() throws Exception { + RecordedRequest lastRequest = runRedirectionTest(302); + // for 302, upon retry, we use the original URI + assertEquals(REQUEST_PATH, lastRequest.getPath()); + } + + public void testNoEtag() throws Exception { + enqueuePartialResponse(5).removeHeader("Etag"); + runSimpleFailureTest(HTTP_LENGTH_REQUIRED); + } + + public void testSanitizeMediaType() throws Exception { + enqueueEmptyResponse(HTTP_OK).addHeader("Content-Type", "text/html; charset=ISO-8859-4"); + Download download = enqueueRequest(getRequest()); + download.runUntilStatus(DownloadManager.STATUS_SUCCESSFUL); + assertEquals("text/html", download.getStringField(DownloadManager.COLUMN_MEDIA_TYPE)); + } + + public void testNoContentLength() throws Exception { + enqueueEmptyResponse(HTTP_OK).removeHeader("Content-Length"); + runSimpleFailureTest(HTTP_LENGTH_REQUIRED); + } + + public void testNoContentType() throws Exception { + enqueueResponse(HTTP_OK, "", false); + runSimpleFailureTest(HTTP_NOT_ACCEPTABLE); + } + + public void testInsufficientSpace() throws Exception { + // this would be better done by stubbing the system API to check available space, but in the + // meantime, just use an absurdly large header value + enqueueEmptyResponse(HTTP_OK).addHeader("Content-Length", + 1024L * 1024 * 1024 * 1024 * 1024); + runSimpleFailureTest(DownloadManager.ERROR_INSUFFICIENT_SPACE); + } + + public void testCancel() throws Exception { + enqueuePartialResponse(5); + Download download = enqueueRequest(getRequest()); + download.runUntilStatus(DownloadManager.STATUS_PAUSED); + + mManager.remove(download.mId); + mSystemFacade.incrementTimeMillis(RETRY_DELAY_MILLIS); + startService(null); + Thread.sleep(500); // TODO: eliminate this when we can run the service synchronously + } + + private void runSimpleFailureTest(int expectedErrorCode) throws Exception { + Download download = enqueueRequest(getRequest()); + download.runUntilStatus(DownloadManager.STATUS_FAILED); + assertEquals(expectedErrorCode, + download.getLongField(DownloadManager.COLUMN_ERROR_CODE)); + } + + /** + * Run a redirection test consisting of + * 1) Request to REQUEST_PATH with 3xx response redirecting to another URI + * 2) Request to REDIRECTED_PATH with interrupted partial response + * 3) Resume request to complete download + * @return the last request sent to the server, resuming after the interruption + */ + private RecordedRequest runRedirectionTest(int status) + throws MalformedURLException, Exception { + enqueueEmptyResponse(status).addHeader("Location", + mServer.getUrl(REDIRECTED_PATH).toString()); + enqueueInterruptedDownloadResponses(5); + + Download download = enqueueRequest(getRequest()); + RecordedRequest request = download.runUntilStatus(DownloadManager.STATUS_PAUSED); + assertEquals(REQUEST_PATH, request.getPath()); + + mSystemFacade.incrementTimeMillis(RETRY_DELAY_MILLIS); + request = download.runUntilStatus(DownloadManager.STATUS_PAUSED); + assertEquals(REDIRECTED_PATH, request.getPath()); + + mSystemFacade.incrementTimeMillis(RETRY_DELAY_MILLIS); + request = download.runUntilStatus(DownloadManager.STATUS_SUCCESSFUL); + return request; + } + private DownloadManager.Request getRequest() throws MalformedURLException { return getRequest(getServerUri(REQUEST_PATH)); } -- cgit v1.2.3