From d6d22e913bba4bf35c7a5f84debb001e23421bc1 Mon Sep 17 00:00:00 2001 From: Qiming Yuan Date: Fri, 20 Jan 2017 18:47:47 -0800 Subject: [PATCH] Optimize upload memory usage. --- Dropbox.Api.Tests/DropboxApiTests.cs | 87 +++++++++++++++++- Dropbox.Api.Tests/MockHttpMessageHandler.cs | 17 ++-- Dropbox.Api/DropboxRequestHandler.cs | 99 +++++++++++---------- 3 files changed, 144 insertions(+), 59 deletions(-) diff --git a/Dropbox.Api.Tests/DropboxApiTests.cs b/Dropbox.Api.Tests/DropboxApiTests.cs index 82ed0f8df2..acea8cde48 100644 --- a/Dropbox.Api.Tests/DropboxApiTests.cs +++ b/Dropbox.Api.Tests/DropboxApiTests.cs @@ -24,6 +24,11 @@ namespace Dropbox.Api.Tests [TestClass] public class DropboxApiTests { + /// + /// The user access token. + /// + public static string UserAccessToken; + /// /// The Dropbox client. /// @@ -39,11 +44,12 @@ public class DropboxApiTests /// public static DropboxAppClient AppClient; + [ClassInitialize] public static void Initialize(TestContext context) { - var userToken = context.Properties["userAccessToken"].ToString(); - Client = new DropboxClient(userToken); + UserAccessToken = context.Properties["userAccessToken"].ToString(); + Client = new DropboxClient(UserAccessToken); var teamToken = context.Properties["teamAccessToken"].ToString(); TeamClient = new DropboxTeamClient(teamToken); @@ -123,6 +129,42 @@ public async Task TestUpload() Assert.AreEqual("abc", content); } + /// + /// Test upload with retry. + /// + /// The + [TestMethod] + public async Task TestUploadRetry() + { + var count = 0; + + var mockHandler = new MockHttpMessageHandler((r, s) => + { + if (count++ < 2) + { + var error = new HttpResponseMessage(HttpStatusCode.InternalServerError) + { + Content = new StringContent("Error") + }; + + return Task.FromResult(error); + } + + return s(r); + }); + + var mockClient = new HttpClient(mockHandler); + var client = new DropboxClient( + UserAccessToken, + new DropboxClientConfig { HttpClient = mockClient, MaxRetriesOnError = 10 }); + + var response = await client.Files.UploadAsync("/Foo.txt", body: GetStream("abc")); + var downloadResponse = await Client.Files.DownloadAsync("/Foo.txt"); + var content = await downloadResponse.GetContentAsStringAsync(); + Assert.AreEqual("abc", content); + } + + /// /// Test upload. /// @@ -154,7 +196,7 @@ public async Task TestRateLimit() mockResponse.Headers.Add("X-Dropbox-Request-Id", "123"); - var mockHandler = new MockHttpMessageHandler(mockResponse); + var mockHandler = new MockHttpMessageHandler((r, s) => Task.FromResult(mockResponse)); var mockClient = new HttpClient(mockHandler); var client = new DropboxClient("dummy", new DropboxClientConfig { HttpClient = mockClient }); try @@ -275,5 +317,44 @@ private static MemoryStream GetStream(string content) var buffer = Encoding.UTF8.GetBytes(content); return new MemoryStream(buffer); } + + /// Test User-Agent header is set with default values. + /// + /// The + [TestMethod] + public async Task TestUserAgentDefault() + { + HttpRequestMessage lastRequest = null; + var mockHandler = new MockHttpMessageHandler((r, s) => + { + lastRequest = r; + return s(r); + }); + + var mockClient = new HttpClient(mockHandler); + var client = new DropboxClient(UserAccessToken, new DropboxClientConfig { HttpClient = mockClient }); + await client.Users.GetCurrentAccountAsync(); + Assert.IsTrue(lastRequest.Headers.UserAgent.ToString().Contains("OfficialDropboxDotNetSDKv2")); + } + + /// Test User-Agent header is populated with user supplied value in DropboxClientConfig. + /// + /// The + [TestMethod] + public async Task TestUserAgentUserSupplied() + { + HttpRequestMessage lastRequest = null; + var mockHandler = new MockHttpMessageHandler((r, s) => + { + lastRequest = r; + return s(r); + }); + + var mockClient = new HttpClient(mockHandler); + var userAgent = "UserAgentTest"; + var client = new DropboxClient(UserAccessToken, new DropboxClientConfig { HttpClient = mockClient, UserAgent = userAgent }); + await client.Users.GetCurrentAccountAsync(); + Assert.IsTrue(lastRequest.Headers.UserAgent.ToString().Contains(userAgent)); + } } } diff --git a/Dropbox.Api.Tests/MockHttpMessageHandler.cs b/Dropbox.Api.Tests/MockHttpMessageHandler.cs index 82b521bd0b..21f210fe63 100644 --- a/Dropbox.Api.Tests/MockHttpMessageHandler.cs +++ b/Dropbox.Api.Tests/MockHttpMessageHandler.cs @@ -6,24 +6,27 @@ namespace Dropbox.Api.Tests { + using System; using System.Net.Http; using System.Threading; using System.Threading.Tasks; - public class MockHttpMessageHandler : HttpMessageHandler + public class MockHttpMessageHandler : HttpClientHandler { + public delegate Task Sender(HttpRequestMessage message); + /// - /// The fake response. + /// The mock handler. /// - private readonly HttpResponseMessage response; - + Func> handler; + /// /// Initializes a new instance of the class. /// /// The mock response. - public MockHttpMessageHandler(HttpResponseMessage response) + public MockHttpMessageHandler(Func> handler) { - this.response = response; + this.handler = handler; } /// @@ -34,7 +37,7 @@ public MockHttpMessageHandler(HttpResponseMessage response) /// The response. protected override Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) { - return Task.FromResult(this.response); + return this.handler(request, r => base.SendAsync(r, cancellationToken)); } } } diff --git a/Dropbox.Api/DropboxRequestHandler.cs b/Dropbox.Api/DropboxRequestHandler.cs index ddb16fe4cb..556db4986e 100644 --- a/Dropbox.Api/DropboxRequestHandler.cs +++ b/Dropbox.Api/DropboxRequestHandler.cs @@ -238,9 +238,6 @@ private async Task RequestJsonStringWithRetry( var maxRetries = this.options.MaxClientRetries; var r = new Random(); - byte[] cachedBody = null; - long cachedStreamStart = 0; - if (routeStyle == RouteStyle.Upload) { if (body == null) @@ -254,66 +251,50 @@ private async Task RequestJsonStringWithRetry( { maxRetries = 0; } - else if (maxRetries == 0) - { - // Do not copy the stream - } - else if (body is MemoryStream) - { - cachedStreamStart = body.Position; - cachedBody = ((MemoryStream)body).ToArray(); - } - else - { - cachedStreamStart = body.Position; - using (var mem = new MemoryStream()) - { - await body.CopyToAsync(mem).ConfigureAwait(false); - cachedBody = mem.ToArray(); - } - } } - while (true) + try { - try + while (true) { - if (cachedBody == null) + try { return await this.RequestJsonString(host, routeName, auth, routeStyle, requestArg, body) .ConfigureAwait(false); } - else + catch (RateLimitException) { - using (var mem = new MemoryStream(cachedBody, writable: false)) - { - mem.Position = cachedStreamStart; - return await this.RequestJsonString(host, routeName, auth, routeStyle, requestArg, mem) - .ConfigureAwait(false); - } + throw; } - } - catch (RateLimitException) - { - throw; - } - catch (RetryException) - { - // dropbox maps 503 - ServiceUnavailable to be a rate limiting error. - // do not count a rate limiting error as an attempt - if (++attempt > maxRetries) + catch (RetryException) { - throw; + // dropbox maps 503 - ServiceUnavailable to be a rate limiting error. + // do not count a rate limiting error as an attempt + if (++attempt > maxRetries) + { + throw; + } } - } - // use exponential backoff - var backoff = TimeSpan.FromSeconds(Math.Pow(2, attempt) * r.NextDouble()); + // use exponential backoff + var backoff = TimeSpan.FromSeconds(Math.Pow(2, attempt) * r.NextDouble()); #if PORTABLE40 - await TaskEx.Delay(backoff); + await TaskEx.Delay(backoff); #else - await Task.Delay(backoff); + await Task.Delay(backoff); #endif + if (body != null) + { + body.Position = 0; + } + } + } + finally + { + if (body != null) + { + body.Dispose(); + } } } @@ -412,7 +393,7 @@ private async Task RequestJsonString( throw new ArgumentNullException("body"); } - request.Content = new StreamContent(body); + request.Content = new CustomStreamContent(body); request.Content.Headers.ContentType = new MediaTypeHeaderValue("application/octet-stream"); break; default: @@ -641,6 +622,26 @@ public void Dispose() } } + /// + /// The stream content which doesn't dispose the underlying stream. This + /// is useful for retry. + /// + internal class CustomStreamContent : StreamContent + { + /// + /// Initializes a new instance of the class. + /// + /// The stream content. + public CustomStreamContent(Stream content) : base(content) + { + } + + protected override void Dispose(bool disposing) + { + // Do not dispose the stream. + } + } + /// /// The type of api hosts. /// @@ -752,7 +753,7 @@ public DropboxRequestHandlerOptions( this.UserAgent = userAgent == null ? string.Join("/", BaseUserAgent, sdkVersion) - : string.Join("/", this.UserAgent, BaseUserAgent, sdkVersion); + : string.Join("/", userAgent, BaseUserAgent, sdkVersion); this.HttpClient = httpClient ?? DefaultHttpClient; this.OAuth2AccessToken = oauth2AccessToken;