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

Some videos have deleted or moved streams #36

Closed
BrunoZell opened this Issue Jul 1, 2017 · 16 comments

Comments

Projects
None yet
2 participants
@BrunoZell
Copy link
Contributor

BrunoZell commented Jul 1, 2017

When using some specific videos I get a System.Net.Http.HttpRequestException comming from the YoutubeClient.GetContentLengthAsync method. More specifically the implemented HttpService throws because the http status code is 401.

Here are some example videos:

Haven't made out what's so special about them.

@Tyrrrz

This comment has been minimized.

Copy link
Owner

Tyrrrz commented Jul 1, 2017

I get
Error 404 -> Video 1,2,4
Error 410 -> Video 3,5,6,7
Looks like some of the streams were deleted for whatever reason but the references still remain in the get_video_info API.
The only thing the videos seem to have in common is that they all have very few available qualities - either 144p and 240p or only 360p.

@Tyrrrz

This comment has been minimized.

Copy link
Owner

Tyrrrz commented Jul 1, 2017

I really don't want to add try/catch blocks to the HTTP requests... Maybe we can just add a IHttpService.PingAsync(string url) and use it to see if the stream is alive?

@Tyrrrz Tyrrrz added the bug label Jul 1, 2017

@Tyrrrz Tyrrrz changed the title YoutubeClient.GetContentLengthAsync throws System.Net.Http.HttpRequestException Some videos have deleted or moved streams Jul 1, 2017

@BrunoZell

This comment has been minimized.

Copy link
Contributor Author

BrunoZell commented Jul 1, 2017

Actually, instead of using response.EnsureSuccessStatusCode(); we can maually check if response.IsSuccessStatusCode and then act accordingly. I'd say just drop the stream (since it's not available anymore), but I haven't decided on what's the best implementation of this.

@Tyrrrz

This comment has been minimized.

Copy link
Owner

Tyrrrz commented Jul 1, 2017

You mean like returning null from those methods if the code doesn't signify success? The problem is then IHttpService cannot guarantee all its implementation will follow that guideline. We can also just return HttpResponse to callers but I feel like it's an overkill, considering this is (so far) the only place where this would be needed. I think it's fine that they throw exceptions and then we can use bool CheckSuccessAsync(string url) only when we actually expect some url not to work (but still able to continue execution).

@BrunoZell

This comment has been minimized.

Copy link
Contributor Author

BrunoZell commented Jul 1, 2017

I do belive we can't get arround a (specific) try-catch, since

  1. As you said, we don't need to implement magic return values or out parameters
  2. Thats the purpose of those exceptions, to handle exceptions
@Tyrrrz

This comment has been minimized.

Copy link
Owner

Tyrrrz commented Jul 1, 2017

Where exactly and what exception are you suggesting to catch?

@BrunoZell

This comment has been minimized.

Copy link
Contributor Author

BrunoZell commented Jul 1, 2017

Oh, I was wrong. I thought the http status code is in the exception data included.
When do you mean we should execute bool CheckSuccessAsync(string url)? Befor every time we execute IHttpService.GetHeadersAsync?

@Tyrrrz

This comment has been minimized.

Copy link
Owner

Tyrrrz commented Jul 1, 2017

No, just for the streams I think

@BrunoZell

This comment has been minimized.

Copy link
Contributor Author

BrunoZell commented Jul 1, 2017

That's what I mean, currently it's every time ^^ But now I see what you are going for. I think you are talking about the mixed stream for loop in it's whole.
Hm, could be possible to do so

@Tyrrrz

This comment has been minimized.

Copy link
Owner

Tyrrrz commented Jul 1, 2017

Oh yeah, I meant inside that loop instead of inside GetHeadersAsync or GetContentLengthAsync

@BrunoZell

This comment has been minimized.

Copy link
Contributor Author

BrunoZell commented Jul 1, 2017

But I'm still not sure about that exception design. I think there are many more error sources on some videos out there, we never can cover all posibilities. But when an error occured and from the GetVideoInfoAsync(id)-method just comes a HttpRequestException for example, I as library user would have no idea what to do to fix it. I think there should be at least a hint or something that's saying in which part of processing an error occured.

@Tyrrrz

This comment has been minimized.

Copy link
Owner

Tyrrrz commented Jul 1, 2017

The thing is, if they are not using the default HttpService they might have a different exception and then YoutubeClient won't be able to handle it. :/

@Tyrrrz

This comment has been minimized.

Copy link
Owner

Tyrrrz commented Jul 1, 2017

For example, if someone implements WebClientHttpService : IHttpService that uses WebClient instead of HttpClient, which would then throw WebException instead of HttpRequestException

@BrunoZell

This comment has been minimized.

Copy link
Contributor Author

BrunoZell commented Jul 1, 2017

Wouldn't it become an inner exception anyways?

@Tyrrrz

This comment has been minimized.

Copy link
Owner

Tyrrrz commented Jul 1, 2017

Hmm. I meant that doing this:

try
{
    long contentLength = await GetContentLengthAsync(url).ConfigureAwait(false);
}
catch (HttpRequestException)
{
}

...isn't really safe because there's no guarantee the supplied _httpService will actually throw that exact exception on failure.

@Tyrrrz

This comment has been minimized.

Copy link
Owner

Tyrrrz commented Jul 2, 2017

Ok on the second thought, sending CheckSuccessAsync on every stream is probably a bad idea. It's going to significantly increase the time it takes to do GetVideoInfoAsync...

@Tyrrrz Tyrrrz closed this in 98a8698 Jul 3, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment