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

CCurlFile::Exists() to fallback to GET if 405 is returned #14586

Merged
merged 1 commit into from Oct 21, 2018

Conversation

@lobermann
Copy link
Member

commented Oct 12, 2018

Description

I found this issue as as the iPhone App (JSON RPC) did not show any covers that where not cached by kodi.

There can be the case that the webserver does not support the HEAD request.
In this case (return code 405, Method not allowed) we can fall back to a GET request to check
if the file exists. Using the range hander to only return 1 byte of the file if supported by the server.

If there can be other cases where a fallback to GET would be good, let me know.

How Has This Been Tested?

Yes, tested on MacOS.

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • None of the above (please explain below)
@lobermann lobermann requested review from notspiff and peak3d Oct 12, 2018
@peak3d

This comment has been minimized.

Copy link
Contributor

commented Oct 12, 2018

Thx for your contribtion, but we should avoid adding magic at this place.
I introduced some weeks a go the retrieval of the return code, so if any there are parts / programs / addons which wand to call GET after 405, it should be implemented there.

If you have a 1GB file behind your URL, GET is a bad choice for HEAD replacement.
The application may want to add byte ranges ( if server supports it)

@wsnipex

This comment has been minimized.

Copy link
Member

commented Oct 13, 2018

this PR makes sense. Exists is called a gazillion times from everywhere and returning false because the server refuses HEAD requests is a bug.
We already have nearly the same logic(and more) for Stat in

/* some http servers and shoutcast servers don't give us any data on a head request */
/* request normal and just bail out via progress meter callback after we received data */
/* somehow curl doesn't reset CURLOPT_NOBODY properly so reset everything */
SetCommonOptions(m_state);
SetRequestHeaders(m_state);
g_curlInterface.easy_setopt(m_state->m_easyHandle, CURLOPT_TIMEOUT, CServiceBroker::GetSettingsComponent()->GetAdvancedSettings()->m_curlconnecttimeout);
g_curlInterface.easy_setopt(m_state->m_easyHandle, CURLOPT_FILETIME, 1);
#if LIBCURL_VERSION_NUM >= 0x072000 // 0.7.32
g_curlInterface.easy_setopt(m_state->m_easyHandle, CURLOPT_XFERINFOFUNCTION, transfer_abort_callback);
#else
g_curlInterface.easy_setopt(m_state->m_easyHandle, CURLOPT_PROGRESSFUNCTION, transfer_abort_callback);

so we should do the same here

@lobermann lobermann force-pushed the lobermann:curfile_exists_get_fallback branch from 4060651 to 13d7a09 Oct 13, 2018
@lobermann

This comment has been minimized.

Copy link
Member Author

commented Oct 13, 2018

@peak3d I agree that magic behind the scenes can be an issue. But it can also be an issue if people do not implement the correct behaviour.
There is a byte range specified in the GET Request in order to avoid downloads of big files.

@wsnipex thanks for the insight!

I went ahead and updated the code to include the timeout via advancedsettings and the CURLOPT_XFERINFOFUNCTION and CURLOPT_NOPROGRESS from the Stat function.

#if LIBCURL_VERSION_NUM >= 0x072000 // 0.7.32
g_curlInterface.easy_setopt(m_state->m_easyHandle, CURLOPT_XFERINFOFUNCTION, transfer_abort_callback);
#else
g_curlInterface.easy_setopt(m_state->m_easyHandle, CURLOPT_PROGRESSFUNCTION, transfer_abort_callback);

This comment has been minimized.

Copy link
@Rechi

Rechi Oct 13, 2018

Member

This isn't needed anymore.

This comment has been minimized.

Copy link
@lobermann

lobermann Oct 13, 2018

Author Member

ok, thanks. removed.

This comment has been minimized.

Copy link
@wsnipex

wsnipex Oct 13, 2018

Member

I think @Rechi meant the curl version specific line is not needed. the transfer_abort_callback should definitely be used though.

This comment has been minimized.

Copy link
@lobermann

lobermann Oct 15, 2018

Author Member

ok, got that wrong. I added the CURLOPT_XFERINFOFUNCTION back again.

@lobermann lobermann force-pushed the lobermann:curfile_exists_get_fallback branch from 13d7a09 to b066cc3 Oct 13, 2018
There can be the case that the webserver does not support the HEAD
request. In this case we can fall back to a GET request to check
if the file exists. Using the range hander to only return 1 byte
of the file if supported by the server.
@lobermann lobermann force-pushed the lobermann:curfile_exists_get_fallback branch from b066cc3 to 4a0bb57 Oct 15, 2018
@wsnipex wsnipex added the Type: Fix label Oct 18, 2018
@wsnipex

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

@peak3d ok for you?

@peak3d
peak3d approved these changes Oct 18, 2018
@lobermann

This comment has been minimized.

Copy link
Member Author

commented Oct 20, 2018

Thanks @Rechi @wsnipex @peak3d . If there are no more comments I will go ahead and merge it tomorrow evening.

@peak3d

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2018

Yes, cool, thx for contribution!

@lobermann lobermann merged commit ab2abe5 into xbmc:master Oct 21, 2018
1 check passed
1 check passed
default You're awesome. Have a cookie
Details
@Rechi Rechi added the v18 Leia label Oct 21, 2018
@Rechi Rechi added this to the Leia 18.0-beta4 milestone Oct 21, 2018
@lobermann lobermann referenced this pull request Oct 28, 2018
0 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.