Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
The oEmbed resource fetcher is used to retrieve oembed data on-demand by things like field formatters. It does this by making a GET request to the resolved oEmbed resource url:
try {
$response = $this->httpClient->get($url);
}
catch (RequestException $e) {
throw new ResourceException('Could not retrieve the oEmbed resource.', $url, [], $e);
}
No connection timeout is passed to Guzzle, so it will default to what's set in \Drupal\Core\Http\ClientFactory's default initialization of 30 seconds.
If you have a site with a cold cache entry for an oembed resource, and the resource service is down and hanging all connections, you could easily swamp your own site as PHP gets tied up waiting 30 seconds every time someone is trying to view this resource. Especially true if you have multiple embeds from the same service on a single page and/or that page is very popular.
Something similar has happened on sites I've maintained in the past and it's not fun to deal with.
Proposed resolution
Ideally, Drupal would never request the oembed resource as a result of a visitor viewing the site, but that probably expands the scope of this issue too far. In the short term, I think it makes sense to set this value to 5 seconds, and perhaps make it configurable. I don't think an oembed service provided should reasonably take more than 5 seconds and timing out after that mount dramatically reduces the impact of that service being down.
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|
Issue fork drupal-3202145
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
kuldeep_mehra27 CreditAttribution: kuldeep_mehra27 at Valuebound for Valuebound commentedComment #3
kuldeep_mehra27 CreditAttribution: kuldeep_mehra27 at Valuebound for Valuebound commentedComment #4
kuldeep_mehra27 CreditAttribution: kuldeep_mehra27 at Valuebound for Valuebound commentedComment #11
phenaproximaWrote tests and opened a new merge request targeting 9.3.x. I didn't open the 9.2.x one, so I can't close it, but we should close it so that committers can backport this patch at their discretion.
Comment #12
phenaproximaIMHO we should add configurability in a follow-up. The reason being that we'd need an update path to create the new config, and that will need its own test coverage, plus the need for ResourceFetcher to accept a new dependency (the config factory), which requires a deprecation...eurgh. Lots of overhead. Let's punt all that to a follow-up, and mitigate the issue at hand as quickly as possible.
Comment #13
phenaproximaWhoops, wrong tag.
Comment #14
Chris Burge CreditAttribution: Chris Burge at University of Nebraska commentedMR looks good. New unit test fails without updated code, so the test is valid. +1 for committing
Comment #17
catchI think we could make this a single, re-usable, $settings key, similar to
$settings['entity_update_batch_size'].Update module is another case where it shouldn't but theoretically can/does make outgoing http requests during a page request. Opened #3228350: Add a generic $settings timeout for outgoing http requests.
Committed/pushed to 9.3.x and cherry-picked to 9.2.x, thanks!
Comment #18
xjmI think the backport broke 9.2.x on
Drupal\Tests\media\Unit\ResourceFetcherTestE.g:
https://www.drupal.org/pift-ci-job/2150889
Comment #20
catchOK I think it is just a missing use statement in the test (for
NullBackend), but moving back to 9.3.x and fixed having reverted from 9.2.x for now. If someone strongly feels this should be backported, please re-open with a backport patch/MR though.