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

johncowen · 2018-12-11T18:14:03Z

Blocking queries maintains an open connection to the consul API. When the API is using HTTP/1.1 to communicate, connections potentially reach 6 open connections, and as far as we are aware most browsers allow a maximum of 6 HTTP/1.1 connections per domain.

This PR includes various things:

An object pool to 'acquire', 'release' and 'dispose' of objects, also a 'purge' to completely empty it. This language is chosen here to try to associate these things to 'pools', I'm probably 99% happy with these, further suggestions welcome.
A Request data object, mainly for reasoning about the 'connection' object easier.
A pseudo http 'client' which doesn't actually control the request itself but does help to manage the connections.
Functionality for aborting connections if you 'hide' the browser tab (this seems to be changing via changing tabs, hiding/minimising your browser or your computer sleeping). There is nothing here for if you open a new window (as opposed to a tab)

Further details:

We attempt a detection of HTTP/2 on app load, this seems to be pretty well supported using the performance API ( https://developer.mozilla.org/en-US/docs/Web/API/Performance/getEntriesByType ). This is done in a progressive manner so we fallback to HTTP/1.1 if we can't detect - I wrapped this in a try to be sure. We check for other potentially multiplexing protocols here also.
We only abort connections if they are not currently downloading. This is to avoid wasting a download or a large amount of services/items when you open a new tab mid-download.
We make use of another private method to achieve this, this seems to be pretty much the only way to do this. This is another reason why I'd like to write a new HTTPAdapter (see UI: Maintain HTTP Headers from all API requests as JSON-API meta data #4946 )
Although we think 6 is the connection limit, we limit to 5 connections to make sure we keep a connection for things we can't manage via javascript (HTML and CSS files etc). Please note, if you really really try you can make the connections fill up on a HTTP/1.1 connection by opening lots of tabs and going to certain pages. This is relatively unlikely and is only likely at all once we enable blocking queries support in the UI. Once we do this, we will also be adding a user setting to turn blocking queries on/off if desired.
Further work would include syncing the connection pool between tabs so that we can manage connections between tabs even if you have opened a new window instead of just a tab. This is currently out of scope but is something we've been looking at and would potentially use our work from UI: Add EventSource ready for implementing blocking queries #5070

This is also mentioned in #5070 and will be used for connection management for blocking queries.

We decided not to do HTTP connection management for users accessing the API over HTTP/2 (and similar multiplexed protocols). Some of our users will be accessing the API via a TLS protected HTTP/2 enabled proxy and therefore shouldn’t have any sort of connection limits applied.
When using HTTP/2 you could potentially open a large amount of blocking queries with the UI, especially if you have a high wait setting. We queried this with the rest of the consul team to see if there is any restriction on the amount of blocking queries we should open to consul, and there doesn’t seem to be any restrictions, or reason to restrict.
We feel that not cancelling sent requests when the user leaves a page (unless a user as no more connections available when using HTTP/1.1) makes for a better user experience. Partly downloaded data is not wasted and changes that have happened since you left a previous page are immediately visible when you return, it doesn’t require another request to be sent back to the API. Blocking queries will naturally timeout and will not restart if the user isn’t on the page, this means that the amount of open connections will remain reasonably small even when using HTTP/2.

Most of this isn't currently in use, and is to be merged onto ui-staging. But if you can see any more gotchas here feedback would be most welcome.

Last but most definitely not least, big thanks to @DingoEatingFuzz as I wouldn't have even realised we needed to do this if it wasn't for him 🙌

This includes various things: 1. An object pool to 'acquire', 'release' and 'dispose' of objects, also a 'purge' to completely empty it 2. A `Request` data object, mainly for reasoning about the object better 3. A pseudo http 'client' which doens't actually control the request itself but does help to manage the connections

johncowen · 2018-12-12T11:01:03Z

@i0rek just pointed me to this, I've not read it/thought about it completely but its pretty relevant here:

https://tlsdocs.netlify.com/docs/guides/creating-certificates.html#configuring-the-consul-ui-for-https

Also gives me some interesting RFC ideas

DingoEatingFuzz

This is a cool way to handle this problem.

I really wish Ember Data had some sort of abort hook out of the box. Both your solution and my solution to this problem are unfortunately invasive.

I had a lot to say, but nothing blocking.

DingoEatingFuzz · 2019-01-08T23:49:14Z

ui-v2/app/services/client/http.js

+          prev = item.nextHopProtocol;
+        }
+        return prev;
+      }, 'http/1.1');


This probably works just fine given that scripts are built ahead of time and always served from the same host as the api, but it still scares me. Mostly I'm concerned of the last item happening to be http/2 despite some/most/all of the other requests being http/1.1. I think this could only happen if some requests are from external sources, but I don't actually know. Just seems safer to to only override prev if prev isn't already http/1.1, and then start with http/2 or whatever the correct token for that is.

I'm pretty sure the last script is the one that is currently executing. I remember thinking 'which is the best connection to check to fairly reliably guess what protocol the UI is using'. I couldn't wait for an actual API connection as I need the info before then. I think I went for the assumption that the script for the javascript for the UI would 'probably' be using the same protocol as the API. I need to double check what I was doing there really though, thanks for querying.

DingoEatingFuzz · 2019-01-08T23:50:08Z

ui-v2/app/services/client/http.js

+      default:
+        // generally 6 are available
+        // reserve 1 for traffic that we can't manage
+        maxConnections = 5;


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

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

DingoEatingFuzz · 2019-01-08T23:51:18Z

ui-v2/app/services/client/http.js

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


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

Pretty sure maxConnections can only be undefined or a number?

DingoEatingFuzz · 2019-01-08T23:57:26Z

ui-v2/app/utils/http/request.js

+    this._headers = {
+      ...headers,
+      'content-type': 'application/json',
+      'x-request-id': `${this._method} ${this._url}?${JSON.stringify(headers.body)}`,


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.

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.

DingoEatingFuzz · 2019-01-09T00:04:05Z

ui-v2/tests/unit/utils/get-object-pool-test.js

+  const pool = getObjectPool(function() {}, 10, actual);
+  pool.acquire(expected, expected.id);
+  assert.deepEqual(actual[0], expected);
+  pool.acquire(expected2, expected2);


You aren't passing in expected2.id as the second argument.

Doh, good catch!

DingoEatingFuzz · 2019-01-09T00:05:28Z

ui-v2/app/utils/get-object-pool.js

@@ -0,0 +1,52 @@
+export default function(dispose = function() {}, max, objects = []) {


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

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?

DingoEatingFuzz · 2019-01-09T00:05:53Z

ui-v2/app/utils/get-object-pool.js

+      // what happens if we can't get an id via getId or .id?
+      // could potentially use Set
+      objects.push(obj);
+      if (typeof max !== 'undefined') {


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

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.

DingoEatingFuzz · 2019-01-09T00:12:26Z

ui-v2/app/utils/get-object-pool.js

@@ -0,0 +1,52 @@
+export default function(dispose = function() {}, max, objects = []) {
+  return {
+    acquire: function(obj, id) {


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.

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.

DingoEatingFuzz · 2019-01-09T00:12:42Z

ui-v2/app/utils/get-object-pool.js

+          itemId = item.id;
+        }
+        if (itemId === id) {
+          index = i;


It'd be nice if you could break here.

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

johncowen · 2019-01-10T14:32:20Z

Thanks for looking at this, agree its not great you have to get a bit low level in the ember scheme of things, hopefully a HTTPAdapter can avoid that eventually. I need to look into a few of the comments you've made here, but I'll answer off the top of my head for now and come back again later once I've double checked.

johncowen · 2019-01-24T15:21:28Z

Hey @DingoEatingFuzz

Sorry for the delay on this, I'm just starting to come back around here now.

I had a little bit of a deeper look into the protocol sniffing, and it wasn't detecting the current script that we were running in (so consul-ui.js), and after your comment I realised this is probably what I wanted to do. The previous approach is 'probably' fine, but what I've done here I think will be better.

So, I added the traditional 'get currently running script' technique, but in order to do this, you have to do it in an initializer outside of the initialize function (anything else in ember must run after a nextTick of some sort, so it gives you the wrong <script>).

There are probably a few ways to pass this 'current script' in, I just went for the most straightforwards for me (add a method to the class to check), I could have used a setter/getter etc, but figured as I'm only using the value once this seemed like the most appropriate approach. If you have any other ideas that might be better lemme know, happy to switch it out for something else.

johncowen · 2019-01-25T12:29:32Z

Hey @DingoEatingFuzz

I'm going to merge this, please feel free to add further comment, its going onto ui-staging so there's still opportunity to amend before going onto master

Thanks,

John

Adds xhr connection managment to http/1.1 installs This includes various things: 1. An object pool to 'acquire', 'release' and 'dispose' of objects, also a 'purge' to completely empty it 2. A `Request` data object, mainly for reasoning about the object better 3. A pseudo http 'client' which doens't actually control the request itself but does help to manage the connections An initializer is used to detect the script element of the consul-ui sourcecode which we use later to sniff the protocol that we are most likely using for API access

johncowen added the theme/ui Anything related to the UI label Dec 11, 2018

johncowen requested a review from a team December 11, 2018 18:14

johncowen mentioned this pull request Dec 11, 2018

UI: Add EventSource ready for implementing blocking queries #5070

Merged

johncowen added this to the 1.4.1 milestone Dec 17, 2018

DingoEatingFuzz approved these changes Jan 9, 2019

View reviewed changes

johncowen modified the milestones: 1.4.1, 1.4.2 Jan 16, 2019

John Cowen added 2 commits January 24, 2019 14:57

ui: Add in missing id property for object pool tests

bf115cf

ui: Add client initializer so we can reliably detect the current script

531c4e7

johncowen modified the milestones: 1.4.2, Upcoming Jan 25, 2019

johncowen merged commit 5970e82 into ui-staging Jan 25, 2019

johncowen deleted the feature/ui-connection-pool branch January 25, 2019 12:30

johncowen mentioned this pull request Feb 15, 2019

UI: Add setting to enable/disable blocking queries #5352

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

johncowen commented Dec 11, 2018 •

edited

Loading

johncowen commented Dec 12, 2018 •

edited

Loading

DingoEatingFuzz left a comment

DingoEatingFuzz Jan 8, 2019

johncowen Jan 10, 2019

DingoEatingFuzz Jan 8, 2019

johncowen Jan 10, 2019

DingoEatingFuzz Jan 8, 2019

johncowen Jan 10, 2019

DingoEatingFuzz Jan 8, 2019

johncowen Jan 10, 2019

DingoEatingFuzz Jan 9, 2019

johncowen Jan 10, 2019

DingoEatingFuzz Jan 9, 2019

johncowen Jan 10, 2019

DingoEatingFuzz Jan 9, 2019

johncowen Jan 10, 2019

DingoEatingFuzz Jan 9, 2019

johncowen Jan 10, 2019

DingoEatingFuzz Jan 9, 2019

johncowen Jan 10, 2019

johncowen commented Jan 10, 2019

johncowen commented Jan 24, 2019

johncowen commented Jan 25, 2019

		@@ -0,0 +1,52 @@
		export default function(dispose = function() {}, max, objects = []) {

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

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

Conversation

johncowen commented Dec 11, 2018 • edited Loading

johncowen commented Dec 12, 2018 • edited Loading

DingoEatingFuzz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johncowen commented Jan 10, 2019

johncowen commented Jan 24, 2019

johncowen commented Jan 25, 2019

johncowen commented Dec 11, 2018 •

edited

Loading

johncowen commented Dec 12, 2018 •

edited

Loading