Fix for User-Agent header duplication on each http request #83
Conversation
|
Thanks! This LGTM at a high level - @kevingilliard can you help review this? |
| @@ -243,7 +249,7 @@ internal BlockingRequestHandler(Client client, TimeSpan timeout) | |||
| // If status code is greater than 500 and less than 600, it indicates server error | |||
| // Error code 429 indicates rate limited. | |||
| // Retry uploading in these cases. | |||
| Task.Delay(backoff).Wait(); | |||
| await Task.Delay(backoff).ConfigureAwait(false); | |||
There was a problem hiding this comment.
Since the MakeRequest method is async using Wait() instead of await can cause deadlock. I can move it to another PR if needed, just thought it is too small for separate one.
There was a problem hiding this comment.
I'd like to be to seperate if you don't mind! It makes it easy to independently revert the header change from this one if needed and also link/attribute properly from the changelog.
| @@ -257,7 +263,7 @@ internal BlockingRequestHandler(Client client, TimeSpan timeout) | |||
| #endif | |||
| } | |||
|
|
|||
| if (backoff == MAXIMUM_BACKOFF_DURATION && statusCode != (int)HttpStatusCode.OK) | |||
| if (backoff >= MAXIMUM_BACKOFF_DURATION || statusCode != (int)HttpStatusCode.OK) | |||
There was a problem hiding this comment.
Can you explain this change?
There was a problem hiding this comment.
backoff is changed only after delay where it is multiplied by 2 each time. The initial value is 100, so it will be 200, 400, 800 and so on. MAXIMUM_BACKOFF_DURATION is set to 10000 so if statement will never be executed. The second part will work in the next situation: if you get response 400 before backoff reached max value then you need to check status code here.
There was a problem hiding this comment.
Got it - could this be a different PR similar to https://github.com/segmentio/Analytics.NET/pull/83/files#r197556500
|
|
||
| if (_client.Config.CompressRequest) | ||
| { | ||
| requestInfo.Add("gzipped json size", requestData.Length); |
There was a problem hiding this comment.
Right, will remove it.
| @@ -36,8 +36,8 @@ | |||
| <Prefer32Bit>false</Prefer32Bit> | |||
| </PropertyGroup> | |||
| <ItemGroup> | |||
| <Reference Include="nunit.framework, Version=3.9.0.0, Culture=neutral, PublicKeyToken=2638cd05610744eb, processorArchitecture=MSIL"> | |||
| <HintPath>..\packages\NUnit.3.9.0\lib\net45\nunit.framework.dll</HintPath> | |||
| <Reference Include="nunit.framework, Version=3.8.1.0, Culture=neutral, PublicKeyToken=2638cd05610744eb, processorArchitecture=MSIL"> | |||
There was a problem hiding this comment.
Curious - why did this change?
There was a problem hiding this comment.
I couldn't build test project because version 3.8.1 was referenced in packages.config. Since the same version is used across all other test projects I just fixed the path here.
|
Hi, @f2prateek |
|
@kevingilliard take a look at the changes and let me or @daletskyi know if you spot any issues with this PR or if it looks good to you :) |
Currently "User-Agent" header is added on each
MakeRequestcall and it's value gets duplicated (docs). If client application usesAnalytics.Clientas a singleton then after some time "User-Agent" header reaches request limits on Segment and it will always return400 Bad Request. This PR moves header initilialization toBlockingRequestHandlerconstructor and also fixes handling of errors.