dispose HttpWebResponse in order to fix problems with timeout after m…

[ptr1120]

Like #27 I also realized problems with timeouts when sending multiple requests. When I send 2 requests in succession the third request always ends with a timeout exception. The suggested solution in #27 fixes this problem but results in a object disposed exception when the consumer access the HttpWebResponse object. Therefore I decided to change the interface of the tracker in order to return just the HttpStatusCode instead of HttpWebResponse. I also do not see any advantages for the consumer to handle with the HttpWebResponse object.

[julienmoumne]

Thanks for looking into this issue.

In general, changing the public signature of a SDK requires some thinking.

This is because current users of the SDK might be relying on the HttpWebResponse being available.

Also, even though the SDK is being used quite extensively, it is the first time this error has been reported.

Could the timeouts you are experiencing be caused by another factor?

In the mean time, would calling Dispose() on the returned HttpWebResponse object be a suitable temporary workaround?

[ptr1120]

Thanks for your answer.

Yes, changing the signature is critical and my PR is just a suggestion for a possible solution. As I said I am unsure for what purpose the consumer should handle with the returned HttpWebResponse. This object mainly contains the HttpStatusCode and a possible answer from the piwik server (ResponseStream). Does the tracking api from the piwik server write anything from interest into the response stream? If this is true another possibillity would be to return a custom poco to the consumer. Although disposing the object fixes the described problem,I am not a friend of dedicating the responsibillity for disposing objects created by this library to the consumer, this can easily result in failures and frustation at consumer side.

We are using the piwik tracker for a desktop application used by approximatly 1000 users around the world. I always realised that not all reqests get submitted to piwik and thought of overload problems at server side because of too many requests.
Recently I tested the piwik tracker more in depth and realised reproducable timeouts when calling e.g. the doBulkTrack method in a loop more than 2 times. After disposing the HttpWebResponse all proplems with timeouts went away. Can't you confirm the timeout problem with the descibed scenario?

Btw changing the signature (away from php tracker api towards usage / compatitibillity of dot net (core)) will be a important future task for the piwik dot net tracker. I would like to have async methods that do not block the program that uses the tracker. Also a possibillty to swallow exception would be a nice feature for users that do not want to deal with exception from tracking e.g. in a productive environment where the internet connction is buggy.

Best regards,
Peter

[julienmoumne]

Thanks for giving this detailed analysis.

I agree that piwik-dotnet-tracker can and will diverge from piwik-php-tracker, as mentioned in #22.

I like the idea of using a custom poco, holding only the status code for a start. This way, if we need to add more fields, the API will remain backward compatible.

Another option would be to use a different library. Would you know of a more modern HTTP library in .NET that would support async, more flexible management of resources and better representation of results?

Whatever we choose to solve the issue, the method signature will change. Except if I am mistaken, this means we will have to bump the major version and release v3.0.0. Do comment on this aspect if you have different thoughts.

[ptr1120]

I agree to you. I would use the HttpClient from System.Net.Http namespace. It is similar to HttpWebRequest and can easily used asynchronously. If I find some time I will try to provide a PR. Shall I change the Method signatures to asynchronous signatures also in this PR or do we want to fix this issue with undisposed HttpWebResponse at fist?

Best regards,
Peter

[julienmoumne]

Thank you for spending time thinking this through.

Would you only provide async methods? Couldn't we design the API to provide both sync & async methods?

If you are willing to work on this and when you have the time, could you comment with code snippets showing how the Piwik API would be used in both cases?

[ptr1120]

Yes, it is a good idea to provide both async and sync methods, like:
public async Task<TrackingResponse> doTrackPageViewAsync(string documentTitle = null) and
public TrackingResponse doTrackPageView(string documentTitle = null)
this is a commonly used pattern.

[julienmoumne]

This looks good.

If you manage to solve the two issues (timeouts & async methods) separately then do provide two PRs/Issues.

Otherwise it's fine to solve both issues in the same PR.

We'll then release v3.0.0 to respect semantic versioning.

I would take care of creating the changelog.

Cheers

[ptr1120]

I have one problem with the asynchronous implementation: the piwik tracker is currently targeting .net framework 4, but asynchronous language support is only available for C# 5 (.net 4.5) and higher.
Would it be a problem to target .net 4.5?
For information: .Net 4.5 is included per default in windows 8, whereas Windows 10 includes .Net 4.6.1 (https://blogs.msdn.microsoft.com/astebner/2007/03/14/mailbag-what-version-of-the-net-framework-is-included-in-what-version-of-the-os/).

[julienmoumne]

Thanks for your work Peter.

Ok to raise requirement to .NET 4.5 for version v3.0.0.

Do you have the capacity to test both improvements on your systems before we release the new package?

[ptr1120]

Thanks Julien. Another minor problem: I forgot to refactor the Piwik.Tracker.Samples solution (PiwikTrackerSamples.displayHttpWebReponse() requires HttpWebResponse as argument).
Can I fix this by adding another short PR. And can I also (in this PR) link the 2 sample projects (Piwik.Tracker.Samples, Piwik.Tracker.Web.Samples) into the the solution Piwik.Tracker (make one solution with 3 projects out of 3 solutions with one project), this would make future refactorings more secure.

Of course I will manually test my implementation but I rely on at least someone else reviewing the code in short and also manually testing some aspects. Additionally I would also like to create a UnitTest Project (with NUnit as Test framework). As we already know unit testing is very difficult due to the nature of this library (depending on a piwik server). I would like to try something with a MockHttp library (see https://github.com/richardszalay/mockhttp), there we can at least verify whether some requests have been send. Maybe someone else has also ideas for testing and then the test library will be extended in near future.

Thanks also for the fast decision for .NET 4.5 for version v3.0.0.

[julienmoumne]

Sorry for not spotting the missing refactoring.
I always open the 2 sample solutions when I perform refactoring and somehow assumed others would as well. This clearly does not make sense. Having only one solution makes much more sense.

Concerning automated tests.

This is definitely an important goal.

I had a look at mockhttp. I failed to see how it could be used to test the tracker as it does not seem to be possible to retrieve a log of the requests as in mock4net#verify-interactions.

If I am mistaken please comment.

As of now, I have two ideas to test the library :

• set-up an embeddable http server (like mock4net) and perform string comparisons on the access log to verify the requests have been received and are well formed (ie. contains the right piwik parameters)
• use docker-piwik to process the tracking requests and piwik-dotnet-api to check if they were correctly understood

If the first idea allows us to reproduce the timeouts you experienced, which it probably can, then the second method is way overkill.

[ptr1120]

Idea 1 is exacly what I want to do (asserting wich urls have been requested) and should be possible with mockhttp. Mock4net looks also interesting, I will have a look on it.

[julienmoumne]

What I also like with Mock4net is that it actually starts a server. This makes me think that we can cover more tests, including the mismanagement of resources such as the one we experienced.

I have not developed with any of this libraries so I am not pushing one in particular.

[ptr1120]

I will try to reconstruct the issue with mismanagement of resources and see whether the test triggers in such a case, this will influence my decision for the mocking library. I also want testing support as much as possible.

Just another idea from my side, please tell me your opinion about it:
We have changed the api now and it was decided to diverge from php tracker as mentioned in #22.
What do you think about including another api change for version 3.0.0 where we apply common .net / c# coding styles / conventions as e.g. seen in https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/coding-style.md.
In this PR I would like to address following changes:

• capitalize all Methods (PascalCasing), e.g. :
  doPing()
  becomes to:
  DoPing()
• Convert getter/setter for instance members to auto-properties:

private string ip;
public void setIp(string ip)
{
      this.ip = ip;
}
public void getIp(string ip)
{
      this.ip = ip;
}

becomes to:
public string Ip {get; set;}

[julienmoumne]

Yes please definitely!

[julienmoumne]

By the way, would you be ok to switch to BSD? If that is the case, please thumps-up in Anthon comment in #23.

[tetsuo13]

I'd love to help out with some of the new issues that have spawned from this PR if possible. Is there any way to assign them out?

[ptr1120]

Yes, from my side every kind of help would be appreciated for the following tasks:

1. Reviewing / discussing of changes for version 3.0.0.
2. Multi platform support
3. UnitTests

Regarding 2.: You could implement the support for e.g. .NetCore, UWP, Mono, Xamarin,... I see big capability for the piwik dot net tracker as a effort able solution for usage tracking. Therefor we should support as many platforms as possible. This task includes the following steps:

• defining the platforms we want to support (see platforms: https://docs.microsoft.com/de-de/dotnet/articles/standard/library). Maybe we can summarize some platforms because of same dependencies (NetStandard).
• creating visual studio projects for the corresponding platforms (e.g. a project Piwik.Tracker.NetStandard) (example see: https://github.com/richardszalay/mockhttp)
• linking the source files into the new project
• fix compiler error by installing needed nuget packages for the platform (e.g. in Piwik dotnet Tracker not compatible with uwp/uap #25 we can see System.Security.Cryptography is not available in UWP)
• document how to create / generate a nuget package corresponding to the nuget specification (see: https://docs.microsoft.com/en-us/nuget/schema/nuspec), especcially putting the platform files in the correct folders (maybe create build script)
• manually short testing consuming the new nuget

Regarding 3.: First I would like to evaluate my test approaches with some tests, then I would need help to get a sufficient test coverage.

[julienmoumne]

Thank you for the vision Peter, I have created issue #35.

[julienmoumne]

Timeout issue fixed in #30