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

Creates a timeout for preconnects #893

Merged
merged 1 commit into from
Nov 18, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 4 additions & 6 deletions builtins/amp-ad.js
Original file line number Diff line number Diff line change
Expand Up @@ -176,8 +176,6 @@ export function installAd(win) {
/** @private {boolean} */
this.shouldSendIntersectionChanges_ = false;

this.prefetchAd_();

if (!this.fallback_) {
this.isDefaultFallback_ = true;

Expand All @@ -189,9 +187,9 @@ export function installAd(win) {

/**
* Prefetches and preconnects URLs related to the ad.
* @private
* @override
*/
prefetchAd_() {
preconnectCallback(onLayout) {
// We always need the bootstrap.
prefetchBootstrap(this.getWin());
const type = this.element.getAttribute('type');
Expand All @@ -205,10 +203,10 @@ export function installAd(win) {
});
}
if (typeof preconnect == 'string') {
this.preconnect.url(preconnect);
this.preconnect.url(preconnect, onLayout);
} else if (preconnect) {
preconnect.forEach(p => {
this.preconnect.url(p);
this.preconnect.url(p, onLayout);
});
}
// If fully qualified src for ad script is specified we preconnect to it.
Expand Down
8 changes: 7 additions & 1 deletion extensions/amp-iframe/0.1/amp-iframe.js
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,13 @@ class AmpIframe extends AMP.BaseElement {
this.transformSrcDoc();
this.iframeSrc = this.assertSource(iframeSrc, window.location.href,
this.element.getAttribute('sandbox'));
this.preconnect.url(this.iframeSrc);
}

/** @override */
preconnectCallback(onLayout) {
if (this.iframeSrc) {
this.preconnect.url(this.iframeSrc, onLayout);
}
}

/** @override */
Expand Down
4 changes: 2 additions & 2 deletions extensions/amp-instagram/0.1/amp-instagram.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@ import {loadPromise} from '../../../src/event-helper';

class AmpInstagram extends AMP.BaseElement {
/** @override */
createdCallback() {
this.preconnect.url('https://instagram.com');
preconnectCallback(onLayout) {
this.preconnect.url('https://instagram.com', onLayout);
}

/** @override */
Expand Down
4 changes: 2 additions & 2 deletions extensions/amp-pinterest/0.1/amp-pinterest.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,9 @@ const BASE60 = '0123456789ABCDEFGHJKLMNPQRSTUVWXYZ_abcdefghijkmnopqrstuvwxyz';

class AmpPinterest extends AMP.BaseElement {
/** @override */
createdCallback() {
preconnectCallback(onLayout) {
// preconnect to widget API
this.preconnect.url('https://widgets.pinterest.com');
this.preconnect.url('https://widgets.pinterest.com', onLayout);
}
/** @override */
isLayoutSupported(layout) {
Expand Down
4 changes: 2 additions & 2 deletions extensions/amp-twitter/0.1/amp-twitter.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@ import {loadPromise} from '../../../src/event-helper';

class AmpTwitter extends AMP.BaseElement {
/** @override */
createdCallback() {
preconnectCallback(onLayout) {
// This domain serves the actual tweets as JSONP.
this.preconnect.url('https://syndication.twitter.com');
this.preconnect.url('https://syndication.twitter.com', onLayout);
// Hosts the script that renders tweets.
this.preconnect.prefetch('https://platform.twitter.com/widgets.js');
prefetchBootstrap(this.getWin());
Expand Down
6 changes: 4 additions & 2 deletions extensions/amp-youtube/0.1/amp-youtube.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,10 @@ import {loadPromise} from '../../../src/event-helper';
class AmpYoutube extends AMP.BaseElement {

/** @override */
createdCallback() {
this.preconnect.url('https://www.youtube.com');
preconnectCallback(onLayout) {
this.preconnect.url('https://www.youtube.com', onLayout);
// Host that YT uses to serve JS needed by player.
this.preconnect.url('https://s.ytimg.com', onLayout);
}

/** @override */
Expand Down
15 changes: 15 additions & 0 deletions src/base-element.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ import {vsyncFor} from './vsync';
* State: <NOT BUILT>
* ||
* || buildCallback
* || preconnectCallback may be called N times after this.
* ||
* \/
* State: <BUILT>
Expand All @@ -76,6 +77,11 @@ import {vsyncFor} from './vsync';
* \/
* State: <IN VIEWPORT>
*
* The preconnectCallback is called when the systems thinks it is good
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add it to the state diagram 😸

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't on purpose.

It isn't part of the state machine. I could put "might be called after this" (as I wrote in the text), but we may call it 100 times at random times and your program should not break.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added to diagram the earliest time when the callback may be called.

* to preconnect to hosts needed by an element. It will never be called
* before buildCallback and it might be called multiple times including
* after layoutCallback.
*
* Additionally whenever the dimensions of an element might have changed
* AMP remeasures its dimensions and calls `onLayoutMeasure` on the
* element instance. This can be used to do additional style calculations
Expand Down Expand Up @@ -205,6 +211,15 @@ export class BaseElement {
// Subclasses may override.
}

/**
* Called by the framework to give the element a chance to preconnect to
* hosts and prefetch resources it is likely to need. May be called
* multiple times because connections can time out.
*/
preconnectCallback() {
// Subclasses may override.
}

/**
* Sets this element as the owner of the specified element. By setting itself
* as an owner, the element declares that it will manage the lifecycle of
Expand Down
11 changes: 11 additions & 0 deletions src/custom-element.js
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,7 @@ export function createAmpElementProto(win, name, implementationClass) {
}
try {
this.implementation_.buildCallback();
this.preconnect(/* onLayout */ false);
this.built_ = true;
this.classList.remove('-amp-notbuilt');
this.classList.remove('amp-notbuilt');
Expand All @@ -434,6 +435,15 @@ export function createAmpElementProto(win, name, implementationClass) {
return true;
};

/**
* Called to instruct the element to preconnect to hosts it uses during
* layout.
* @param {boolean} onLayout Whether this was called after a layout.
*/
ElementProto.preconnect = function(onLayout) {
this.implementation_.preconnectCallback(onLayout);
};

/**
* @return {!Vsync}
* @private
Expand Down Expand Up @@ -659,6 +669,7 @@ export function createAmpElementProto(win, name, implementationClass) {
'Must be upgraded and built to receive viewport events');
this.dispatchCustomEvent('amp:load:start');
const promise = this.implementation_.layoutCallback();
this.preconnect(/* onLayout */ true);
this.classList.add('-amp-layout');
assert(promise instanceof Promise,
'layoutCallback must return a promise');
Expand Down
42 changes: 36 additions & 6 deletions src/preconnect.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ import {parseUrl} from './url';
import {timer} from './timer';
import {platformFor} from './platform';

const ACTIVE_CONNECTION_TIMEOUT_MS = 180 * 1000;
const PRECONNECT_TIMEOUT_MS = 10 * 1000;

class Preconnect {

Expand All @@ -34,9 +36,16 @@ class Preconnect {
constructor(win) {
/** @private @const {!Element} */
this.head_ = win.document.head;
/** @private @const {!Object<string, boolean>} */
/**
* Origin we've preconnected to and when that connection
* expires as a timestamp in MS.
* @private @const {!Object<string, number>}
*/
this.origins_ = {};
/** @private @const {!Object<string, boolean>} */
/**
* Urls we've prefetched.
* @private @const {!Object<string, boolean>}
*/
this.urls_ = {};
/** @private @const {!Platform} */
this.platform_ = platformFor(win);
Expand All @@ -48,16 +57,33 @@ class Preconnect {
* Preconnects to a URL. Always also does a dns-prefetch because
* browser support for that is better.
* @param {string} url
* @param {boolean=} opt_alsoConnecting Set this flag if you also just
* did or are about to connect to this host. This is for the case
* where preconnect is issued immediate before or after actual connect
* and preconnect is used to flatten a deep HTTP request chain.
* E.g. when you preconnect to a host that an embed will connect to
* when it is more fully rendered, you already know that the connection
* will be used very soon.
*/
url(url) {
url(url, opt_alsoConnecting) {
if (!this.isInterestingUrl_(url)) {
return;
}
const origin = parseUrl(url).origin;
if (this.origins_[origin]) {
const now = timer.now();
const lastPreconnectTimeout = this.origins_[origin];
if (lastPreconnectTimeout && now < lastPreconnectTimeout) {
if (opt_alsoConnecting) {
this.origins_[origin] = now + ACTIVE_CONNECTION_TIMEOUT_MS ;
}
return;
}
this.origins_[origin] = true;
// If we are about to use the connection, don't re-preconnect for
// 180 seconds.
const timeout = opt_alsoConnecting
? ACTIVE_CONNECTION_TIMEOUT_MS
: PRECONNECT_TIMEOUT_MS;
this.origins_[origin] = now + timeout;
const dns = document.createElement('link');
dns.setAttribute('rel', 'dns-prefetch');
dns.setAttribute('href', origin);
Expand Down Expand Up @@ -93,7 +119,7 @@ class Preconnect {
return;
}
this.urls_[url] = true;
this.url(url);
this.url(url, /* opt_alsoConnecting */ true);
const prefetch = document.createElement('link');
prefetch.setAttribute('rel', 'prefetch');
prefetch.setAttribute('href', url);
Expand Down Expand Up @@ -133,6 +159,10 @@ class Preconnect {
if (!this.platform_.isSafari()) {
return;
}
// Don't attempt to preconnect for ACTIVE_CONNECTION_TIMEOUT_MS since
// we effectively create an active connection.
// TODO(@cramforce): Confirm actual http2 timeout in Safari.
this.origins_[origin] = timer.now() + ACTIVE_CONNECTION_TIMEOUT_MS;
const url = origin + '/amp_preconnect_polyfill?' + Math.random();
// We use an XHR without withCredentials(true), so we do not send cookies
// to the host and the host cannot set cookies.
Expand Down
11 changes: 11 additions & 0 deletions test/functional/test-custom-element.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ describe('CustomElement', () => {

const resources = resourcesFor(window);
let testElementCreatedCallback;
let testElementPreconnectCallback;
let testElementFirstAttachedCallback;
let testElementBuildCallback;
let testElementLayoutCallback;
Expand All @@ -42,6 +43,9 @@ describe('CustomElement', () => {
createdCallback() {
testElementCreatedCallback();
}
preconnectCallback(onLayout) {
testElementPreconnectCallback(onLayout);
}
firstAttachedCallback() {
testElementFirstAttachedCallback();
}
Expand Down Expand Up @@ -85,6 +89,7 @@ describe('CustomElement', () => {
clock = sandbox.useFakeTimers();

testElementCreatedCallback = sinon.spy();
testElementPreconnectCallback = sinon.spy();
testElementFirstAttachedCallback = sinon.spy();
testElementBuildCallback = sinon.spy();
testElementLayoutCallback = sinon.spy();
Expand Down Expand Up @@ -200,12 +205,14 @@ describe('CustomElement', () => {
expect(res).to.equal(true);
expect(element.isBuilt()).to.equal(true);
expect(testElementBuildCallback.callCount).to.equal(1);
expect(testElementPreconnectCallback.callCount).to.equal(1);

// Call again.
res = element.build(false);
expect(res).to.equal(true);
expect(element.isBuilt()).to.equal(true);
expect(testElementBuildCallback.callCount).to.equal(1);
expect(testElementPreconnectCallback.callCount).to.equal(1);
});

it('Element - build not allowed', () => {
Expand Down Expand Up @@ -387,9 +394,13 @@ describe('CustomElement', () => {
element.build(true);
expect(element.isBuilt()).to.equal(true);
expect(testElementLayoutCallback.callCount).to.equal(0);
expect(testElementPreconnectCallback.callCount).to.equal(1);
expect(testElementPreconnectCallback.getCall(0).args[0]).to.be.false;

const p = element.layoutCallback();
expect(testElementLayoutCallback.callCount).to.equal(1);
expect(testElementPreconnectCallback.callCount).to.equal(2);
expect(testElementPreconnectCallback.getCall(1).args[0]).to.be.true;
return p.then(() => {
expect(element.readyState).to.equal('complete');
});
Expand Down
36 changes: 35 additions & 1 deletion test/functional/test-preconnect.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ describe('preconnect', () => {
});

afterEach(() => {
clock.tick(20000);
clock.tick(200000);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you sure you want 200,000 ms?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. This just makes sure there is nothing left to do.

clock.restore();
sandbox.restore();
});
Expand Down Expand Up @@ -87,6 +87,40 @@ describe('preconnect', () => {
.to.have.length(2);
});

it('should timeout preconnects', () => {
preconnect.url('https://x.preconnect.com/foo/bar');
expect(document.querySelectorAll('link[rel=preconnect]'))
.to.have.length(1);
clock.tick(9000);
preconnect.url('https://x.preconnect.com/foo/bar');
expect(document.querySelectorAll('link[rel=preconnect]'))
.to.have.length(1);
clock.tick(1000);
expect(document.querySelectorAll('link[rel=preconnect]'))
.to.have.length(0);
// After timeout preconnect creates a new tag.
preconnect.url('https://x.preconnect.com/foo/bar');
expect(document.querySelectorAll('link[rel=preconnect]'))
.to.have.length(1);
});

it('should timeout preconnects longer with active connect', () => {
preconnect.url('https://y.preconnect.com/foo/bar',
/* opt_alsoConnecting */ true);
expect(document.querySelectorAll('link[rel=preconnect]'))
.to.have.length(1);
clock.tick(10000);
expect(document.querySelectorAll('link[rel=preconnect]'))
.to.have.length(0);
preconnect.url('https://y.preconnect.com/foo/bar');
expect(document.querySelectorAll('link[rel=preconnect]'))
.to.have.length(0);
clock.tick(180 * 1000);
preconnect.url('https://y.preconnect.com/foo/bar');
expect(document.querySelectorAll('link[rel=preconnect]'))
.to.have.length(1);
});

it('should prefetch', () => {
preconnect.prefetch('https://a.prefetch.com/foo/bar');
preconnect.prefetch('https://a.prefetch.com/foo/bar');
Expand Down