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

ui: Adds XHR connection management to HTTP/1.1 installs #5083

Merged
merged 3 commits into from
Jan 25, 2019
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
25 changes: 24 additions & 1 deletion ui-v2/app/adapters/application.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import Adapter from 'ember-data/adapters/rest';
import { AbortError } from 'ember-data/adapters/errors';
import { inject as service } from '@ember/service';
import { get } from '@ember/object';

import URL from 'url';
import createURL from 'consul-ui/utils/createURL';
import { FOREIGN_KEY as DATACENTER_KEY } from 'consul-ui/models/dc';
import { HEADERS_SYMBOL as HTTP_HEADERS_SYMBOL } from 'consul-ui/utils/http/consul';

export const REQUEST_CREATE = 'createRecord';
export const REQUEST_READ = 'queryRecord';
Expand All @@ -14,10 +16,31 @@ export const REQUEST_DELETE = 'deleteRecord';

export const DATACENTER_QUERY_PARAM = 'dc';

import { HEADERS_SYMBOL as HTTP_HEADERS_SYMBOL } from 'consul-ui/utils/http/consul';
export default Adapter.extend({
namespace: 'v1',
repo: service('settings'),
client: service('client/http'),
manageConnection: function(options) {
const client = get(this, 'client');
const complete = options.complete;
const beforeSend = options.beforeSend;
options.beforeSend = function(xhr) {
if (typeof beforeSend === 'function') {
beforeSend(...arguments);
}
options.id = client.request(options, xhr);
};
options.complete = function(xhr, textStatus) {
client.complete(options.id);
if (typeof complete === 'function') {
complete(...arguments);
}
};
return options;
},
_ajaxRequest: function(options) {
return this._super(this.manageConnection(options));
},
queryRecord: function() {
return this._super(...arguments).catch(function(e) {
if (e instanceof AbortError) {
Expand Down
15 changes: 15 additions & 0 deletions ui-v2/app/initializers/client.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
const scripts = document.getElementsByTagName('script');
const current = scripts[scripts.length - 1];

export function initialize(application) {
const Client = application.resolveRegistration('service:client/http');
Client.reopen({
isCurrent: function(src) {
return current.src === src;
},
});
}

export default {
initialize,
};
87 changes: 87 additions & 0 deletions ui-v2/app/services/client/http.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
import Service, { inject as service } from '@ember/service';
import { get, set } from '@ember/object';
import { Promise } from 'rsvp';

import getObjectPool from 'consul-ui/utils/get-object-pool';
import Request from 'consul-ui/utils/http/request';

const dispose = function(request) {
if (request.headers()['content-type'] === 'text/event-stream') {
const xhr = request.connection();
// unsent and opened get aborted
// headers and loading means wait for it
// to finish for the moment
if (xhr.readyState) {
switch (xhr.readyState) {
case 0:
case 1:
xhr.abort();
break;
}
}
}
return request;
};
export default Service.extend({
dom: service('dom'),
init: function() {
this._super(...arguments);
let protocol = 'http/1.1';
try {
protocol = performance.getEntriesByType('resource').find(item => {
// isCurrent is added in initializers/client and is used
// to ensure we use the consul-ui.js src to sniff what the protocol
// is. Based on the assumption that whereever this script is it's
// likely to be the same as the xmlhttprequests
return item.initiatorType === 'script' && this.isCurrent(item.name);
}).nextHopProtocol;
} catch (e) {
// pass through
}
let maxConnections;
// http/2, http2+QUIC/39 and SPDY don't have connection limits
switch (true) {
case protocol.indexOf('h2') === 0:
case protocol.indexOf('hq') === 0:
case protocol.indexOf('spdy') === 0:
break;
default:
// generally 6 are available
// reserve 1 for traffic that we can't manage
maxConnections = 5;

Choose a reason for hiding this comment

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

+1 to leaving a "channel" open for short-lived requests and unexpected what-have-yous.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hehe, I thought if somebody if somebody was going to use the word 'channel' it was going to be on the EventSource PR!

break;
}
set(this, 'connections', getObjectPool(dispose, maxConnections));
if (typeof maxConnections !== 'undefined') {

Choose a reason for hiding this comment

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

This is truthy when maxConnections is null. That's fine?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pretty sure maxConnections can only be undefined or a number?

set(this, 'maxConnections', maxConnections);
const doc = get(this, 'dom').document();
// when the user hides the tab, abort all connections
doc.addEventListener('visibilitychange', e => {
if (e.target.hidden) {
get(this, 'connections').purge();
}
});
}
},
whenAvailable: function(e) {
const doc = get(this, 'dom').document();
// if we are using a connection limited protocol and the user has hidden the tab (hidden browser/tab switch)
// any aborted errors should restart
if (typeof get(this, 'maxConnections') !== 'undefined' && doc.hidden) {
return new Promise(function(resolve) {
doc.addEventListener('visibilitychange', function listen(event) {
doc.removeEventListener('visibilitychange', listen);
resolve(e);
});
});
}
return Promise.resolve(e);
},
request: function(options, xhr) {
const request = new Request(options.type, options.url, { body: options.data || {} }, xhr);
return get(this, 'connections').acquire(request, request.getId());
},
complete: function() {
return get(this, 'connections').release(...arguments);
},
});
2 changes: 1 addition & 1 deletion ui-v2/app/services/dom.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ export default Service.extend({
},
elementsByTagName: function(name, context) {
context = typeof context === 'undefined' ? get(this, 'doc') : context;
return context.getElementByTagName(name);
return context.getElementsByTagName(name);
},
elements: function(selector, context) {
// don't ever be tempted to [...$$()] here
Expand Down
52 changes: 52 additions & 0 deletions ui-v2/app/utils/get-object-pool.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
export default function(dispose = function() {}, max, objects = []) {

Choose a reason for hiding this comment

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

Seems like an odd design to allow the object array to be provided. It opens up the opportunity for it be mucked with externally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It means I can inspect it whilst testing:

https://github.com/hashicorp/consul/pull/5083/files/4a22886de3d6e75262b4bc71ae5b7b1bec61fb49#diff-876e4c1f4e8e565beb60f501df6c19d1R18

You could pass your own array and then muck with it externally, there are lots of things you can do but shouldn't. I don't think we pass an array in here anywhere in the application code (only during testing). Maybe I should add some comments here to make it clearer?

return {
acquire: function(obj, id) {

Choose a reason for hiding this comment

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

Maybe this is just me being pedantic, but calling this an object pool when objects are provided directly is a misnomer. The core value of a traditional object pool is to prevent allocating and destroying tons of objects. Since this data structure isn't doing any form of object recycling, it isn't really an object pool.

This is still a valuable structure by all means, I'm just not sure what to name it. DisposableSet? DestructorCollection? That one sounds Java as h*ck.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm yeah I'm not 100% sure on the vocabulary here either. The main reason it's not a 'true' pool is that we don't really have the access via ember-data API's currently to be able to do that. Ideally I'd like to turn it into a 'true' pool though, it all depends on a possible future HTTPAdapter and how that might work.

You're right though, the actual functionality here is about disposing/destroying objects so a lot of the vocab works, I think its mainly the filename. Let me rethink it a little, right now I'm tempted to stick to the 'pool' idea as ideally thats what I'd like it to be along with HTTPAdapter, but I'll give it another thought when I go back over.

// TODO: what should happen if an ID already exists
// should we ignore and release both? Or prevent from acquiring? Or generate a unique ID?
// what happens if we can't get an id via getId or .id?
// could potentially use Set
objects.push(obj);
if (typeof max !== 'undefined') {

Choose a reason for hiding this comment

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

An object pool with no max set is just an array...why make setting a max optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose it's a collection of objects that get disposed of by a configurable function. That collection can be constrained to a certain length, or be an infinite length, the thing is the disposal.

if (objects.length > max) {
return dispose(objects.shift());
}
}
return id;
},
// release releases the obj from the pool but **doesn't** dispose it
release: function(obj) {
let index = -1;
let id;
if (typeof obj === 'string') {
id = obj;
} else {
id = obj.id;
}
objects.forEach(function(item, i) {
let itemId;
if (typeof item.getId === 'function') {
itemId = item.getId();
} else {
itemId = item.id;
}
if (itemId === id) {
index = i;

Choose a reason for hiding this comment

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

It'd be nice if you could break here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hehe ah yeah, maybe find here might be better, will check when I go over again.

}
});
if (index !== -1) {
return objects.splice(index, 1)[0];
}
},
purge: function() {
let obj;
const objs = [];
while ((obj = objects.shift())) {
objs.push(dispose(obj));
}
return objs;
},
dispose: function(id) {
return dispose(this.release(id));
},
};
}
29 changes: 29 additions & 0 deletions ui-v2/app/utils/http/request.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
export default class {
constructor(method, url, headers, xhr) {
this._xhr = xhr;
this._url = url;
this._method = method;
this._headers = headers;
this._headers = {
...headers,
'content-type': 'application/json',
'x-request-id': `${this._method} ${this._url}?${JSON.stringify(headers.body)}`,

Choose a reason for hiding this comment

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

Can this header value end up being deceptively large? Either via a long url or a large headers.body value? It might be better to use a uuid generator here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm yeah I suppose a big KV value could get huge. What do you think would be better generating a uuid on a big text blob, or doing a maximum of 5 equality checks on a 5 large KV blobs? I've actually no idea, feels like the latter but wouldn't be surprised if I'm completely wrong there - might have a play and see.

};
if (typeof this._headers.body.index !== 'undefined') {
// this should probably be in a response
this._headers['content-type'] = 'text/event-stream';
}
}
headers() {
return this._headers;
}
getId() {
return this._headers['x-request-id'];
}
abort() {
this._xhr.abort();
}
connection() {
return this._xhr;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,7 @@ import { moduleFor, test } from 'ember-qunit';
import repo from 'consul-ui/tests/helpers/repo';
const NAME = 'intention';
moduleFor(`service:repository/${NAME}`, `Integration | Service | ${NAME}`, {
// Specify the other units that are required for this test.
needs: [
'service:settings',
'service:store',
`adapter:${NAME}`,
`serializer:${NAME}`,
`model:${NAME}`,
],
integration: true,
});

const dc = 'dc-1';
Expand Down
12 changes: 12 additions & 0 deletions ui-v2/tests/unit/services/client/http-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import { moduleFor, test } from 'ember-qunit';

moduleFor('service:client/http', 'Unit | Service | client/http', {
// Specify the other units that are required for this test.
needs: ['service:dom'],
});

// Replace this with your real tests.
test('it exists', function(assert) {
let service = this.subject();
assert.ok(service);
});
98 changes: 98 additions & 0 deletions ui-v2/tests/unit/utils/get-object-pool-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
import getObjectPool from 'consul-ui/utils/get-object-pool';
import { module, skip } from 'qunit';
import test from 'ember-sinon-qunit/test-support/test';

module('Unit | Utility | get object pool');

skip('Decide what to do if you add 2 objects with the same id');
test('acquire adds objects', function(assert) {
const actual = [];
const expected = {
hi: 'there',
id: 'hi-there-123',
};
const expected2 = {
hi: 'there',
id: 'hi-there-456',
};
const pool = getObjectPool(function() {}, 10, actual);
pool.acquire(expected, expected.id);
assert.deepEqual(actual[0], expected);
pool.acquire(expected2, expected2.id);
assert.deepEqual(actual[1], expected2);
});
test('acquire adds objects and returns the id', function(assert) {
const arr = [];
const expected = 'hi-there-123';
const obj = {
hi: 'there',
id: expected,
};
const pool = getObjectPool(function() {}, 10, arr);
const actual = pool.acquire(obj, expected);
assert.equal(actual, expected);
});
test('acquire adds objects, and disposes when there is no room', function(assert) {
const actual = [];
const expected = {
hi: 'there',
id: 'hi-there-123',
};
const expected2 = {
hi: 'there',
id: 'hi-there-456',
};
const dispose = this.stub()
.withArgs(expected)
.returnsArg(0);
const pool = getObjectPool(dispose, 1, actual);
pool.acquire(expected, expected.id);
assert.deepEqual(actual[0], expected);
pool.acquire(expected2, expected2.id);
assert.deepEqual(actual[0], expected2);
assert.ok(dispose.calledOnce);
});
test('it disposes', function(assert) {
const arr = [];
const expected = {
hi: 'there',
id: 'hi-there-123',
};
const expected2 = {
hi: 'there',
id: 'hi-there-456',
};
const dispose = this.stub().returnsArg(0);
const pool = getObjectPool(dispose, 2, arr);
const id = pool.acquire(expected, expected.id);
assert.deepEqual(arr[0], expected);
pool.acquire(expected2, expected2.id);
assert.deepEqual(arr[1], expected2);
const actual = pool.dispose(id);
assert.ok(dispose.calledOnce);
assert.equal(arr.length, 1, 'object was removed from array');
assert.deepEqual(actual, expected, 'returned object is expected object');
assert.deepEqual(arr[0], expected2, 'object in the pool is expected object');
});
test('it purges', function(assert) {
const arr = [];
const expected = {
hi: 'there',
id: 'hi-there-123',
};
const expected2 = {
hi: 'there',
id: 'hi-there-456',
};
const dispose = this.stub().returnsArg(0);
const pool = getObjectPool(dispose, 2, arr);
pool.acquire(expected, expected.id);
assert.deepEqual(arr[0], expected);
pool.acquire(expected2, expected2.id);
assert.deepEqual(arr[1], expected2);
const actual = pool.purge();
assert.ok(dispose.calledTwice, 'dispose was called on everything');
assert.equal(arr.length, 0, 'the pool is empty');
assert.deepEqual(actual[0], expected, 'the first purged object is correct');
assert.deepEqual(actual[1], expected2, 'the second purged object is correct');
});
10 changes: 10 additions & 0 deletions ui-v2/tests/unit/utils/http/request-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import httpRequest from 'consul-ui/utils/http/request';
import { module, test } from 'qunit';

module('Unit | Utility | http/request');

// Replace this with your real tests.
test('it works', function(assert) {
const actual = httpRequest;
assert.ok(typeof actual === 'function');
});