WebConnectionStream.Length now consistently throws a NotSupportedException.

[mlaily]

The behaviour of the Length implementation was not consistent with the expected and documented behaviour of this property:
CanSeek always returns false, so Length should always throw an exception.
In all cases, it should never return -1, as it was the case when the Content-Length header could not be parsed.

See this bug on the Xamarin bug tracker for the discussion and an example of why the previous behaviour was a problem. https://bugzilla.xamarin.com/show_bug.cgi?id=15671

[marek-safar]

Please write unit test for this change

[jonpryor]

@marek-safar This patch effectively reverts commit 4b72b767.

I'm personally find and dandy with that idea; however, I'm leery of reverting that patch because I don't know why that patch was introduced in the first place, nor do I know what (broken) code may be depending on it. (Which is why this needs a test case, to ensure that we don't break other parts of System.Net by doing this.)

Do you have any idea why gonazalo added that change? I don't see him on IRC atm.

[marek-safar]

@jonpryor I have no idea why gonzalo made that change. We probably have to start again and made unit tests for this change and see what breaks if anything

[mlaily]

I'm not sure how I'm supposed to write a test for this class, being a web stream where the result might change based on the http request being send... (And the class is private. Wouldn't that be a problem?)

I guess I could just check whether the Length always throws an exception when CanSeek is false (which is currently always the case) but if you have better ideas...

Anyway, I won't be able to look into this before a few days at least.

[jonpryor]

you'd create a new HttpWebRequest, call HttpWebRequest.GetResponseStream(), and assert that stream.Length throws NotSupportedException.

[knocte]

FYI it's possible that @gonzalop committed 4b72b767 to make sure that an exception is not thrown when calling CheckIfForceWrite() (see 8e67b8c ). Now, the latter commit points to a bug which has restricted access (https://bugzilla.novell.com/show_bug.cgi?id=77753) so I'm guessing that writing a unit test for it would be hard.

[knocte]

Oops, second link is wrong, it should be this: 8e67b8c (edited comment to fix it too).

[jonpryor]

This is why we can't have nice things: I don't see any unit tests for either commit 4b72b76 or 8e67b8c, so we can't know what they're trying to handle and reason about this properly. :-(

[knocte]

Oh, actually 8e67b8c contains a reference to https://bugzilla.xamarin.com/show_bug.cgi?id=1512 and in that bug there is a testcase it seems, so maybe it could be converted to a unit test?

[lewurm]

is there still interest to merge this or should it be closed?

[monojenkins]

Hello! I'm the build bot for the Mono project. I need approval from a Mono team member to build this pull request. A team member should reply with "approve" to approve a build of this pull request, "whitelist" to whitelist this and all future pull requests from this contributor, or "build" to explicitly request a build, even if one has already been done. Contributors can ignore this message.

[migueldeicaza]

It looks like the discussion on the bug https://bugzilla.xamarin.com/show_bug.cgi?id=15671 and https://bugzilla.xamarin.com/show_bug.cgi?id=15672 makes the requirement debatable.