Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use HttpClient and change to async method #54

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

huan086
Copy link
Contributor

@huan086 huan086 commented Apr 20, 2017

Upgrade to .NET 4.6.1. Fixes #34.
Use HttpClient for requests.
Change all tracking methods to async. Fixes #39.

Warning: this pull request removes all the sync methods

@huan086 huan086 changed the title Upgrade to .NET 4.6.1. Fixes #34. Use HttpClient and change to async method Apr 20, 2017
@ptr1120
Copy link
Collaborator

ptr1120 commented Apr 21, 2017

Hi huan086,

you PR looks very good. One problem I see is the requirement for .net 4.6.1 since this would break the usage of the tracker for many people using .Net 4.5. Is there any reason why you upgraded to 4.6.1? I think async support was introduced in .Net 4.5..
Making the HttpClient as a constructor parameter is a good idea for people who are using the HttpClient as singleton in their application, would it be possible to make it as a optional parameter?

Best regards,
Peter

@huan086
Copy link
Contributor Author

huan086 commented Apr 21, 2017

For the .NET 4.6.1, I re-read the deprecation notice for .NET 4.5 and realized I had a misunderstanding. Misunderstand that we shouldn't target .NET 4.5. But the advice was to upgrade the installed framework. Changing the target now.

@julienmoumne
Copy link
Member

julienmoumne commented Apr 21, 2017

Could you elaborate why we shouldn't target .NET 4.5? And why should we update the .NET version?

I don't have much knowledge of the .NET eco system, I am trying to catch-up with you guys. If you have a link to an article with must know facts, please share it here.

Also, is there a way to know how many users won't be able to use the library any more if we update the version? Is there some public per version usage percentages so we can base the decision on some more or less concrete facts?

@huan086 huan086 force-pushed the HttpClientAndAsync branch from e1c6368 to ad6b238 Compare April 21, 2017 10:13
@huan086
Copy link
Contributor Author

huan086 commented Apr 21, 2017

Changed to target .NET 4.5. Added constructor without HttpClient parameter.

@huan086 huan086 force-pushed the HttpClientAndAsync branch from ad6b238 to dee8333 Compare April 21, 2017 10:22
Use HttpClient for requests.
Change all tracking methods to async. Fixes matomo-org#39.
@huan086 huan086 force-pushed the HttpClientAndAsync branch from dee8333 to 510e708 Compare April 21, 2017 10:23
@julienmoumne
Copy link
Member

Thanks for this PR.

I have to take some time to understand the implication of updating the .NET version.

If you have some helpful resources, do share.

@huan086
Copy link
Contributor Author

huan086 commented Apr 21, 2017

Probably see this? https://en.wikipedia.org/wiki/.NET_Framework#Release_history

If anyone is building desktop app, the installer for the app can skip installing .NET Framework 4.5 when on Windows 8 and above. When installing in Windows 7, installation would be longer.

Let's ignore Vista and previous version since they are no longer supported by MS

@julienmoumne
Copy link
Member

We will release v3 before merging this PR.

This will allow users that may be stuck with .NET v4.0 on the production server to benefit from the recent bug fixes and the deprecation of the GPL license.

@huan086
Copy link
Contributor Author

huan086 commented Apr 24, 2017

Since this is a breaking change, it would be ideal if #25 (another breaking change) could be addressed before v4 RTM. However, I have no experience in UWP and .NET Standard. Hopefully someone experienced would help

Copy link
Member

@julienmoumne julienmoumne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just had the time to review this PR.

This is indeed a very good addition to the project.

I have added a few comments, you'll see it's mostly so that I understand better the new async feature.

Thanks!

@@ -585,10 +606,10 @@ protected string GetCookieName(string cookieName)
/// </summary>
/// <param name="documentTitle">Page title as it will appear in the Actions > Page titles report</param>
/// <returns>HTTP Response from the server or null if using bulk requests.</returns>
public TrackingResponse DoTrackPageView(string documentTitle = null)
public Task<TrackingResponse> TrackPageViewAsync(string documentTitle = null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you confirm postfixing every method name with "Async" is either

  • required by the compiler, or
  • a naming standard we should follow because developers expect async methods to end with this keyword?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -721,11 +742,11 @@ public TrackingResponse DoTrackEcommerceCartUpdate(double grandTotal)
/// Response
/// </returns>
/// <exception cref="System.InvalidOperationException">Error: you must call the function DoTrackPageView or DoTrackGoal from this class, before calling this method DoBulkTrack()</exception>
public TrackingResponse DoBulkTrack()
public async Task<TrackingResponse> BulkTrackAsync()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am having difficulties understanding why the keywoard "async" is sometimes used and sometimes omitted in method declarations.

Could you help me understand maybe by providing a link?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

async is required when there is an await in the method body.

For the rest of the methods, due to SendRequestAsync being the last statement in the method, we could avoid having to await on it and return the Task instead (Task is similar to promise in other languages). By doing so, we can also avoid having to decide whether to add .ConfigureAwait(false) or not. (.ConfigureAwait(false) is always good when no synchronization context is involved, but may cause extra thread switches when synchronization context is needed. See discussion)

Copy link
Contributor Author

@huan086 huan086 May 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this explanation is dealing with JavaScript, the underlying way async await works is the same. (C# doesn't yet provide generator, so it's not so good to explain in C#). Languages with async await includes F#, C#, JavaScript and Python. IMO all the rest of the languages are behind time

@@ -738,7 +759,7 @@ public TrackingResponse DoBulkTrack()
}

var postData = new JavaScriptSerializer().Serialize(data);
var response = SendRequest(PiwikBaseUrl, "POST", postData, true);
var response = await SendRequestAsync(PiwikBaseUrl, "POST", postData, true).ConfigureAwait(false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we "await" here, is "BulkTrackAsync" really asynchronous?

My apologies if I am missing something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In .NET, await and async doesn't necessary mean asynchronous :gasp: 😲. One reason to use it is of course to make your code asynchronous, by running several Async methods without awaiting, then subsequently awaiting on the Task.

Here, we only have one SendRequestAsync method, so there's no asynchronous to talk about. However, async is still useful here. SendRequest does a network call, in which traditional synchronous code would have the thread doing nothing, only waiting for the network card to send out the packets, remote server to process, and network card to receive the packets.

SendRequestAsync which calls HttpClient.SendAsync, would instead release the thread to the threadpool (or continue it's event loop if it's the UI thread) at the await. When the packets have been received, the underlying .NET runtime (similar to JVM in Java) would then take a thread from the threadpool and continue the execution, setting our response variable.

The advantage is that the freed up threadpool thread can process other things, instead of consuming memory, handles etc but doing nothing

OR

The freed up UI thread can continue processing events, handling button clicks, process WM_PAINT to prevent "freeze", etc

@@ -1724,7 +1748,9 @@ protected void SetCookie(string cookieName, string cookieValue, long cookieTtl)
{
return new Dictionary<string, string[]>();
}
return new JavaScriptSerializer().Deserialize<Dictionary<string, string[]>>(HttpUtility.UrlDecode(cookie.Value ?? string.Empty));

return new JavaScriptSerializer().Deserialize<Dictionary<string, string[]>>(HttpUtility.UrlDecode(cookie.Value ?? string.Empty)) ??
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain what was the issue you encountered here? Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deserialize returns null when deserializing string.Empty. The sample code does not check for null when using GetCustomVariablesFromCookie.

@@ -60,22 +59,22 @@ private static void Main(string[] args)
/// <summary>
/// Triggers a Goal conversion
/// </summary>
static private void GoalConversion()
private static async Task GoalConversionAsync()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain why the method does not return "void" since nothing is returned? How come it still compiles?

Copy link
Contributor Author

@huan086 huan086 May 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the weirdness of the async await syntax. A promise Task is always created and returned. async void is a NO NO, causing the caller not to receive the promise, and thus not able to await on it, thus not able to ensure the order of execution

@huan086
Copy link
Contributor Author

huan086 commented May 10, 2017

I realized I haven't added CancelationToken prameter to the async methods. Drawback of that is that the caller won't be able to cancel the async task, i.e. abort the HttpClient.SendAsync before it's complete. Will probably do another pull request on top of this

@julienmoumne
Copy link
Member

Thank you for taking the time to educate me on the async support in .NET

I will perform a last review next week and merge your PR.

It would indeed be good to have the possibility to cancel the task. I don't know how much effort it would require though.

Concerning v4, I feel it's OK to release it with only the async support. This is except if the API changes introduced for UWP would require a developer two substantially modify the code when going from v4 to v5.

Copy link
Member

@julienmoumne julienmoumne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for your explanations.

I have a few more questions if you will.


DisplayDebugInfo(response);
}

/// <summary>
/// Records a simple page view with a specified document title
/// </summary>
static private void RecordSimplePageView()
private static async Task RecordSimplePageViewAsync()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took the time to get more familiar with .NET async support.

Method calls in private static void Main(string[] args) have not been renamed, probably because they are commented.

When I uncomment and rename RecordSimplePageView to RecordSimplePageViewAsync in Main(), the compiler warns the keyword await should be used. As soon as the keyword await is added, the compiler warns the Main() method should be async, which is not possible. Is this expected?

Also, if I don't add the await keyword and I shutdown the Piwik server, no exception is reported. Is this expected? How one should handle exceptions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've uncommented the methods and made them work using Task.Run().GetAwaiter().GetResult(). Usually doing so is a bad idea due to possible deadlock. However, it is ok for console app, as the Main method doesn't run in the thread pool, thus doesn't need to compete for available threads (if I'm not wrong).

@julienmoumne
Copy link
Member

Thank you

I have created #66 which I will address to ease the process of refactoring sample code.

@MERamadan
Copy link

Any updates about this PR? I think updating the framework is very important to get the security updates. In addition to, .NET 4.6.1 is compatible with .NET Standard projects. The current version shows the following warning in Visual Studio

[Warning][NU1701] Package 'Piwik.Tracker 3.0.0' was restored using '.NETFramework,Version=v4.6.1' instead of the project target framework '.NETStandard,Version=v2.0'. This package may not be fully compatible with your project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide asynchronous tracking support Deprecate .NET 4.0 to support async methods
4 participants