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: Polling Step 3 - Close connections when tabbing away #3953

Merged
merged 7 commits into from
Mar 8, 2018

Conversation

DingoEatingFuzz
Copy link
Contributor

Since browsers only support ~6 open connections to the same host at once, it's important to eagerly close connections to allow other tabs to resolve requests.

Example problem scenario:

  1. Visit /clients in Tab A
  2. /clients has n + 1 connections, 1 for the nodes list and n for the allocations of each node
  3. Open a client in a new tab (Tab B)
  4. Tab B is in a perpetual loading state because Tab A has already hit the maximum allowed open connections for the nomad server host

Solved problem scenario:

  1. Visit /clients in Tab A
  2. /clients has n + 1 connections, 1 for the nodes list and n for the allocations of each node
  3. Open a client in a new tab (Tab B)
  4. Once Tab B is active, the n + 1 connections in Tab A are all aborted
  5. Tab B loads immediately because there are currently 0 open connections to the nomad server host


export default Mixin.create({
setupDocumentVisibility: function() {
this.set('_visibilityHandler', this.get('visibilityHandler').bind(this));
Copy link
Contributor

Choose a reason for hiding this comment

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

Will you ever have > 1 component with this mixin on a screen? Would you have a last one wins situation here?

Copy link
Member

Choose a reason for hiding this comment

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

I think all components (or routes) would have their handlers run, right? Which is likely the desired behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes to both! Job row and node row both mix this mixin in, so multiple components. Yes all components own their own handler and independently add and remove their own handlers. This prevents accidental side effects when paginating or sorting resulting in one component being destroyed and bringing down all the visibility handlers.


export default Mixin.create({
export default Mixin.create(WithVisibilityDetection, {
Copy link
Contributor

Choose a reason for hiding this comment

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

mixin the mixin 🐢🐢🐢

},

startWatchers() {
assert('startWatchers needs to be overridden in the Route', false);
Copy link
Contributor

Choose a reason for hiding this comment

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

❤ little usage helpers like this


export default Mixin.create({
setupDocumentVisibility: function() {
this.set('_visibilityHandler', this.get('visibilityHandler').bind(this));
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess I have the same question about nested routes as I do with multiple components - do outer routes have their handler overwritten by child routes 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.

Nope, independent handlers again.

Copy link
Member

@alisdair alisdair left a comment

Choose a reason for hiding this comment

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

This is super cool! 😎

On the new mixins: there's an implicit dependency on the visibilityHandler method on the object using the mixin. This feels a little bit tricky to me. Is there any reasonable way of making that dependency more explicit?

@DingoEatingFuzz
Copy link
Contributor Author

Is there any reasonable way of making that dependency more explicit?

Good call. Something like what I did with the default startWatchers in the other mixin?

startWatchers() {
assert('startWatchers needs to be overridden in the Route', false);
},

@alisdair
Copy link
Member

alisdair commented Mar 8, 2018

Yeah, the assert approach seems great to me! 👍

Copy link
Contributor

@meirish meirish left a comment

Choose a reason for hiding this comment

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

Cool 👍! Fun that it was a relatively small change too.

@DingoEatingFuzz DingoEatingFuzz merged commit 73ca228 into f-ui-polling Mar 8, 2018
@DingoEatingFuzz DingoEatingFuzz deleted the f-ui-polling-disconnect branch March 8, 2018 20:51
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants