From 4f892d696acf51de9ed208315972d5b58c25096b Mon Sep 17 00:00:00 2001 From: Zoe Roux Date: Fri, 3 Jan 2025 18:43:56 +0100 Subject: [PATCH 1/2] feat: Add optional cancellation token for oauth client --- .../Clients/IObservableOauthClient.cs | 19 ++++++++++------- .../Clients/ObservableOauthClient.cs | 17 ++++++++------- Octokit/Clients/IOAuthClient.cs | 19 ++++++++++------- Octokit/Clients/OAuthClient.cs | 21 +++++++++++-------- 4 files changed, 45 insertions(+), 31 deletions(-) diff --git a/Octokit.Reactive/Clients/IObservableOauthClient.cs b/Octokit.Reactive/Clients/IObservableOauthClient.cs index 5313caa819..895c9006de 100644 --- a/Octokit.Reactive/Clients/IObservableOauthClient.cs +++ b/Octokit.Reactive/Clients/IObservableOauthClient.cs @@ -1,4 +1,5 @@ using System; +using System.Threading; namespace Octokit.Reactive { @@ -22,8 +23,9 @@ public interface IObservableOauthClient /// an access token using this method. /// /// + /// /// - IObservable CreateAccessToken(OauthTokenRequest request); + IObservable CreateAccessToken(OauthTokenRequest request, CancellationToken cancellationToken = default); /// /// Makes a request to initiate the device flow authentication. @@ -33,25 +35,28 @@ public interface IObservableOauthClient /// This request also returns a device verification code that you must use to receive an access token to check the status of user authentication. /// /// + /// /// - IObservable InitiateDeviceFlow(OauthDeviceFlowRequest request); + IObservable InitiateDeviceFlow(OauthDeviceFlowRequest request, CancellationToken cancellationToken = default); /// - /// Makes a request to get an access token using the response from . + /// Makes a request to get an access token using the response from . /// /// /// Will poll the access token endpoint, until the device and user codes expire or the user has successfully authorized the app with a valid user code. /// /// The client Id you received from GitHub when you registered the application. - /// The response you received from + /// The response you received from + /// /// - IObservable CreateAccessTokenForDeviceFlow(string clientId, OauthDeviceFlowResponse deviceFlowResponse); + IObservable CreateAccessTokenForDeviceFlow(string clientId, OauthDeviceFlowResponse deviceFlowResponse, CancellationToken cancellationToken = default); /// - /// Makes a request to get an access token using the refresh token returned in . + /// Makes a request to get an access token using the refresh token returned in . /// /// Token renewal request. + /// /// with the new token set. - IObservable CreateAccessTokenFromRenewalToken(OauthTokenRenewalRequest request); + IObservable CreateAccessTokenFromRenewalToken(OauthTokenRenewalRequest request, CancellationToken cancellationToken = default); } } diff --git a/Octokit.Reactive/Clients/ObservableOauthClient.cs b/Octokit.Reactive/Clients/ObservableOauthClient.cs index 1e4f42ef66..4d58da79fe 100644 --- a/Octokit.Reactive/Clients/ObservableOauthClient.cs +++ b/Octokit.Reactive/Clients/ObservableOauthClient.cs @@ -1,5 +1,6 @@ using System; using System.Reactive.Threading.Tasks; +using System.Threading; namespace Octokit.Reactive { @@ -23,24 +24,24 @@ public Uri GetGitHubLoginUrl(OauthLoginRequest request) return _client.Oauth.GetGitHubLoginUrl(request); } - public IObservable CreateAccessToken(OauthTokenRequest request) + public IObservable CreateAccessToken(OauthTokenRequest request, CancellationToken cancellationToken = default) { - return _client.Oauth.CreateAccessToken(request).ToObservable(); + return _client.Oauth.CreateAccessToken(request, cancellationToken).ToObservable(); } - public IObservable InitiateDeviceFlow(OauthDeviceFlowRequest request) + public IObservable InitiateDeviceFlow(OauthDeviceFlowRequest request, CancellationToken cancellationToken = default) { - return _client.Oauth.InitiateDeviceFlow(request).ToObservable(); + return _client.Oauth.InitiateDeviceFlow(request, cancellationToken).ToObservable(); } - public IObservable CreateAccessTokenForDeviceFlow(string clientId, OauthDeviceFlowResponse deviceFlowResponse) + public IObservable CreateAccessTokenForDeviceFlow(string clientId, OauthDeviceFlowResponse deviceFlowResponse, CancellationToken cancellationToken = default) { - return _client.Oauth.CreateAccessTokenForDeviceFlow(clientId, deviceFlowResponse).ToObservable(); + return _client.Oauth.CreateAccessTokenForDeviceFlow(clientId, deviceFlowResponse, cancellationToken).ToObservable(); } - public IObservable CreateAccessTokenFromRenewalToken(OauthTokenRenewalRequest request) + public IObservable CreateAccessTokenFromRenewalToken(OauthTokenRenewalRequest request, CancellationToken cancellationToken = default) { - return _client.Oauth.CreateAccessTokenFromRenewalToken(request) + return _client.Oauth.CreateAccessTokenFromRenewalToken(request, cancellationToken) .ToObservable(); } } diff --git a/Octokit/Clients/IOAuthClient.cs b/Octokit/Clients/IOAuthClient.cs index c1add05062..0ce00e4648 100644 --- a/Octokit/Clients/IOAuthClient.cs +++ b/Octokit/Clients/IOAuthClient.cs @@ -1,4 +1,5 @@ using System; +using System.Threading; using System.Threading.Tasks; namespace Octokit @@ -26,8 +27,9 @@ public interface IOauthClient /// an access token using this method. /// /// + /// /// - Task CreateAccessToken(OauthTokenRequest request); + Task CreateAccessToken(OauthTokenRequest request, CancellationToken concellationToken = default); /// /// Makes a request to initiate the device flow authentication. @@ -37,25 +39,28 @@ public interface IOauthClient /// This request also returns a device verification code that you must use to receive an access token to check the status of user authentication. /// /// + /// /// - Task InitiateDeviceFlow(OauthDeviceFlowRequest request); + Task InitiateDeviceFlow(OauthDeviceFlowRequest request, CancellationToken cancellationToken = default); /// - /// Makes a request to get an access token using the response from . + /// Makes a request to get an access token using the response from . /// /// /// Will poll the access token endpoint, until the device and user codes expire or the user has successfully authorized the app with a valid user code. /// /// The client Id you received from GitHub when you registered the application. - /// The response you received from + /// The response you received from + /// /// - Task CreateAccessTokenForDeviceFlow(string clientId, OauthDeviceFlowResponse deviceFlowResponse); + Task CreateAccessTokenForDeviceFlow(string clientId, OauthDeviceFlowResponse deviceFlowResponse, CancellationToken concellationToken = default); /// - /// Makes a request to get an access token using the refresh token returned in . + /// Makes a request to get an access token using the refresh token returned in . /// /// Token renewal request. + /// /// with the new token set. - Task CreateAccessTokenFromRenewalToken(OauthTokenRenewalRequest request); + Task CreateAccessTokenFromRenewalToken(OauthTokenRenewalRequest request, CancellationToken concellationToken = default); } } diff --git a/Octokit/Clients/OAuthClient.cs b/Octokit/Clients/OAuthClient.cs index 9d051d1716..c8616cdd2b 100644 --- a/Octokit/Clients/OAuthClient.cs +++ b/Octokit/Clients/OAuthClient.cs @@ -1,6 +1,7 @@ using System; using System.Globalization; using System.Net.Http; +using System.Threading; using System.Threading.Tasks; namespace Octokit @@ -48,7 +49,7 @@ public Uri GetGitHubLoginUrl(OauthLoginRequest request) } [ManualRoute("POST", "/login/oauth/access_token")] - public async Task CreateAccessToken(OauthTokenRequest request) + public async Task CreateAccessToken(OauthTokenRequest request, CancellationToken cancellationToken = default) { Ensure.ArgumentNotNull(request, nameof(request)); @@ -56,12 +57,12 @@ public async Task CreateAccessToken(OauthTokenRequest request) var body = new FormUrlEncodedContent(request.ToParametersDictionary()); - var response = await connection.Post(endPoint, body, "application/json", null, hostAddress).ConfigureAwait(false); + var response = await connection.Post(endPoint, body, "application/json", null, hostAddress, cancellationToken).ConfigureAwait(false); return response.Body; } [ManualRoute("POST", "/login/device/code")] - public async Task InitiateDeviceFlow(OauthDeviceFlowRequest request) + public async Task InitiateDeviceFlow(OauthDeviceFlowRequest request, CancellationToken cancellationToken = default) { Ensure.ArgumentNotNull(request, nameof(request)); @@ -69,12 +70,12 @@ public async Task InitiateDeviceFlow(OauthDeviceFlowReq var body = new FormUrlEncodedContent(request.ToParametersDictionary()); - var response = await connection.Post(endPoint, body, "application/json", null, hostAddress).ConfigureAwait(false); + var response = await connection.Post(endPoint, body, "application/json", null, hostAddress, cancellationToken).ConfigureAwait(false); return response.Body; } [ManualRoute("POST", "/login/oauth/access_token")] - public async Task CreateAccessTokenForDeviceFlow(string clientId, OauthDeviceFlowResponse deviceFlowResponse) + public async Task CreateAccessTokenForDeviceFlow(string clientId, OauthDeviceFlowResponse deviceFlowResponse, CancellationToken cancellationToken = default) { Ensure.ArgumentNotNullOrEmptyString(clientId, nameof(clientId)); Ensure.ArgumentNotNull(deviceFlowResponse, nameof(deviceFlowResponse)); @@ -85,9 +86,11 @@ public async Task CreateAccessTokenForDeviceFlow(string clientId, Oa while (true) { + cancellationToken.ThrowIfCancellationRequested(); + var request = new OauthTokenRequestForDeviceFlow(clientId, deviceFlowResponse.DeviceCode); var body = new FormUrlEncodedContent(request.ToParametersDictionary()); - var response = await connection.Post(endPoint, body, "application/json", null, hostAddress).ConfigureAwait(false); + var response = await connection.Post(endPoint, body, "application/json", null, hostAddress, cancellationToken).ConfigureAwait(false); if (response.Body.Error != null) { @@ -103,7 +106,7 @@ public async Task CreateAccessTokenForDeviceFlow(string clientId, Oa throw new ApiException(string.Format(CultureInfo.InvariantCulture, "{0}: {1}\n{2}", response.Body.Error, response.Body.ErrorDescription, response.Body.ErrorUri), null); } - await Task.Delay(TimeSpan.FromSeconds(pollingDelay)); + await Task.Delay(TimeSpan.FromSeconds(pollingDelay), cancellationToken); } else { @@ -113,14 +116,14 @@ public async Task CreateAccessTokenForDeviceFlow(string clientId, Oa } [ManualRoute("POST", "/login/oauth/access_token")] - public async Task CreateAccessTokenFromRenewalToken(OauthTokenRenewalRequest request) + public async Task CreateAccessTokenFromRenewalToken(OauthTokenRenewalRequest request, CancellationToken cancellationToken = default) { Ensure.ArgumentNotNull(request, nameof(request)); var endPoint = ApiUrls.OauthAccessToken(); var body = new FormUrlEncodedContent(request.ToParametersDictionary()); - var response = await connection.Post(endPoint, body, "application/json", null, hostAddress).ConfigureAwait(false); + var response = await connection.Post(endPoint, body, "application/json", null, hostAddress, cancellationToken).ConfigureAwait(false); return response.Body; } } From 7c6c08f86114c2e452524f8ca1c9b670443b3857 Mon Sep 17 00:00:00 2001 From: Todd Grunke Date: Wed, 8 Jan 2025 08:56:18 -0800 Subject: [PATCH 2/2] fix: Reduce string allocations during SimpleJson.ParseString (#2977) Noticed this allocation while looking at a profile of solution load in visual studio. These StringBuilder allocations were showing up as 0.5% of total allocations. I took a peek at the code, and reaslized the StringBuilder need not be created unless there is a '\' in the string value being parsed. In that case, just copy already seen characters into a StringBuilder and continue as previously. Co-authored-by: Nick Floyd <139819+nickfloyd@users.noreply.github.com> --- Octokit.Tests/SimpleJsonTests.cs | 36 ++++++++++++++++++++++++++++++++ Octokit/SimpleJson.cs | 25 ++++++++++++++++++---- 2 files changed, 57 insertions(+), 4 deletions(-) create mode 100644 Octokit.Tests/SimpleJsonTests.cs diff --git a/Octokit.Tests/SimpleJsonTests.cs b/Octokit.Tests/SimpleJsonTests.cs new file mode 100644 index 0000000000..ffc211cee4 --- /dev/null +++ b/Octokit.Tests/SimpleJsonTests.cs @@ -0,0 +1,36 @@ +using Octokit; +using System.Threading.Tasks; +using Xunit; + +public class SimpleJsonTests +{ + [Theory] + [InlineData("\"abc\"", "abc")] + [InlineData(" \"abc\" ", "abc")] + [InlineData("\" abc \" ", " abc ")] + [InlineData("\"abc\\\"def\"", "abc\"def")] + [InlineData("\"abc\\r\\ndef\"", "abc\r\ndef")] + public async Task ParseStringSuccess(string input, string expected) + { + int index = 0; + bool success = true; + + string actual = SimpleJson.ParseString(input.ToCharArray(), ref index, ref success); + + Assert.True(success); + Assert.Equal(expected, actual); + } + + [Theory] + [InlineData("\"abc")] + public async Task ParseStringIncomplete(string input) + { + int index = 0; + bool success = true; + + string actual = SimpleJson.ParseString(input.ToCharArray(), ref index, ref success); + + Assert.False(success); + Assert.Null(actual); + } +} diff --git a/Octokit/SimpleJson.cs b/Octokit/SimpleJson.cs index 7b9a855481..bafb26f921 100644 --- a/Octokit/SimpleJson.cs +++ b/Octokit/SimpleJson.cs @@ -792,15 +792,18 @@ static object ParseValue(char[] json, ref int index, ref bool success) return null; } - static string ParseString(char[] json, ref int index, ref bool success) + internal static string ParseString(char[] json, ref int index, ref bool success) { - StringBuilder s = new StringBuilder(BUILDER_CAPACITY); + // Avoid allocating this StringBuilder unless a backslash is encountered in the json + StringBuilder s = null; char c; EatWhitespace(json, ref index); // " c = json[index++]; + + int startIndex = index; bool complete = false; while (!complete) { @@ -815,6 +818,13 @@ static string ParseString(char[] json, ref int index, ref bool success) } else if (c == '\\') { + if (s == null) + { + s = new StringBuilder(BUILDER_CAPACITY); + for (int i = startIndex; i < index - 1; i++) + s.Append(json[i]); + } + if (index == json.Length) break; c = json[index++]; @@ -875,14 +885,21 @@ static string ParseString(char[] json, ref int index, ref bool success) } } else - s.Append(c); + { + if (s != null) + s.Append(c); + } } if (!complete) { success = false; return null; } - return s.ToString(); + + if (s != null) + return s.ToString(); + + return new string(json, startIndex, index - startIndex - 1); } private static string ConvertFromUtf32(int utf32)