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

send longer cache headers by default #1725

Merged
merged 15 commits into from
Jul 20, 2018
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
2 changes: 1 addition & 1 deletion frontend/components/usage.js
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ export default class Usage extends React.PureComponent {
<td>
<code>?maxAge=3600</code>
</td>
<td>Set the HTTP cache lifetime in secs</td>
<td>Set the HTTP cache lifetime in secs (values below the default will be ignored)</td>
</tr>
</tbody>
</table>
Expand Down
4 changes: 4 additions & 0 deletions lib/all-badge-examples.js
Original file line number Diff line number Diff line change
Expand Up @@ -2221,6 +2221,10 @@ function loadExamples() {
loadServiceClasses().forEach(ServiceClass => {
const category = findCategory(ServiceClass.category);
if (category === undefined) {
if (ServiceClass.category === 'debug') {
// we don't want to show debug services on the examples page
return;
}
throw Error(`Unknown category ${ServiceClass.category} referenced in ${ServiceClass.name}`);
}
const prepared = ServiceClass.prepareExamples();
Expand Down
23 changes: 17 additions & 6 deletions lib/request-handler.js
Original file line number Diff line number Diff line change
Expand Up @@ -79,13 +79,24 @@ function handleRequest (makeBadge, handlerOptions) {
return (queryParams, match, end, ask) => {
const reqTime = new Date();

if (queryParams.maxAge !== undefined && /^[0-9]+$/.test(queryParams.maxAge)) {
ask.res.setHeader('Cache-Control', 'max-age=' + queryParams.maxAge);
ask.res.setHeader('Expires', new Date(+reqTime + queryParams.maxAge * 1000).toGMTString());
} else {
// Cache management - no cache, so it won't be cached by GitHub's CDN.
let maxAge = parseInt(process.env.BADGE_MAX_AGE_SECONDS) || 0;
if (
queryParams.maxAge !== undefined
&& /^[0-9]+$/.test(queryParams.maxAge)
&& parseInt(queryParams.maxAge) > maxAge
) {
// only queryParams.maxAge to override the default
// if it is greater than env.BADGE_MAX_AGE_SECONDS
maxAge = parseInt(queryParams.maxAge);
}
// send both Cache-Control max-age and Expires
// in case the client implements HTTP/1.0 but not HTTP/1.1
if (maxAge === 0) {
ask.res.setHeader('Cache-Control', 'no-cache, no-store, must-revalidate');
ask.res.setHeader('Expires', reqTime.toGMTString()); // Proxies, GitHub, see #221.
ask.res.setHeader('Expires', reqTime.toGMTString());
} else {
ask.res.setHeader('Cache-Control', 'max-age=' + maxAge);
ask.res.setHeader('Expires', new Date(+reqTime + maxAge * 1000).toGMTString());
Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason to set Expires along with Cache-Control: max-age? To my reading of the spec, Cache-Control: max-age takes priority.

Copy link
Member

Choose a reason for hiding this comment

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

It's explained here #1651 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for that pointer!

From #1651 (comment):

It may be pointless in a perfect world but different implementations take different approach, there will always be someone not following the spec or maybe also not supporting Cache-Control but supporting Expires.

It seems like we're acting on a kind of superstition. It would be great to have an example of a client which does this, or a reliable report that this is true.

I'm fine leaving it in for now, though let's add a reference that stays with the code.

Copy link
Member Author

Choose a reason for hiding this comment

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

The Cache-Control header was defined as part of the HTTP/1.1 specification would be ignored by any HTTP client which implements the HTTP/1.0 standard but not HTTP/1.1. This is definitely an edge case and we should expect any modern web browser or downstream cache to implement HTTP/1.1 but we can't safely assume that only clients implementing HTTP/1.1 will connect to us. I've added a comment clarifying this.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for explaining that!

}

ask.res.setHeader('Date', reqTime.toGMTString());
Expand Down
36 changes: 33 additions & 3 deletions lib/request-handler.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ describe('The request handler', function() {
before(analytics.load);

let camp;
const initialBadgeMaxAge = process.env.BADGE_MAX_AGE_SECONDS;

beforeEach(function (done) {
camp = Camp.start({ port: config.port, hostname: '::' });
camp.on('listening', () => done());
Expand All @@ -43,6 +45,7 @@ describe('The request handler', function() {
camp.close(() => done());
camp = null;
}
process.env.BADGE_MAX_AGE_SECONDS = initialBadgeMaxAge;
});

describe('the options object calling style', function() {
Expand Down Expand Up @@ -105,15 +108,42 @@ describe('The request handler', function() {
expect(handlerCallCount).to.equal(1);
});

it('should set the expires header to current time', async function () {
it('should set the expires header to current time + BADGE_MAX_AGE_SECONDS', async function () {
process.env.BADGE_MAX_AGE_SECONDS = 900;
const res = await fetch(`${baseUri}/testing/123.json`);
expect(res.headers.get('expires')).to.equal(res.headers.get('date'));
const expectedExpiry = new Date(+(new Date(res.headers.get('date'))) + 900000).toGMTString();
expect(res.headers.get('expires')).to.equal(expectedExpiry);
expect(res.headers.get('cache-control')).to.equal('max-age=900');
});

it('should set the expires header to current time + max-age', async function () {
it('should set the expires header to current time + maxAge', async function () {
process.env.BADGE_MAX_AGE_SECONDS = 0;
const res = await fetch(`${baseUri}/testing/123.json?maxAge=3600`);
const expectedExpiry = new Date(+(new Date(res.headers.get('date'))) + 3600000).toGMTString();
expect(res.headers.get('expires')).to.equal(expectedExpiry);
expect(res.headers.get('cache-control')).to.equal('max-age=3600');
});

it('should ignore maxAge if maxAge < BADGE_MAX_AGE_SECONDS', async function () {
process.env.BADGE_MAX_AGE_SECONDS = 600;
const res = await fetch(`${baseUri}/testing/123.json?maxAge=300`);
const expectedExpiry = new Date(+(new Date(res.headers.get('date'))) + 600000).toGMTString();
expect(res.headers.get('expires')).to.equal(expectedExpiry);
expect(res.headers.get('cache-control')).to.equal('max-age=600');
});

it('should set Cache-Control: no-cache, no-store, must-revalidate if maxAge=0', async function () {
process.env.BADGE_MAX_AGE_SECONDS = 0;
const res = await fetch(`${baseUri}/testing/123.json`);
expect(res.headers.get('expires')).to.equal(res.headers.get('date'));
expect(res.headers.get('cache-control')).to.equal('no-cache, no-store, must-revalidate');
});

it('should set Cache-Control: no-cache, no-store, must-revalidate if BADGE_MAX_AGE_SECONDS not set', async function () {
delete process.env.BADGE_MAX_AGE_SECONDS;
const res = await fetch(`${baseUri}/testing/123.json`);
expect(res.headers.get('expires')).to.equal(res.headers.get('date'));
expect(res.headers.get('cache-control')).to.equal('no-cache, no-store, must-revalidate');
});

describe('the cache key', function () {
Expand Down
31 changes: 31 additions & 0 deletions services/time/time.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
'use strict';

const BaseService = require('../base');

module.exports = class Time extends BaseService {

async handle() {
return { message: new Date() };
}

// Metadata
static get defaultBadgeData() {
return {
label: 'time',
color: 'blue',
};
}

static get category() {
return 'debug';
}

static get url() {
return {
base: 'servertime',
format: '',
capture: []
};
}

};