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

[17.09] Client side fixes for GIEs #4680

Merged
merged 7 commits into from Sep 27, 2017

Conversation

Projects
None yet
6 participants
@natefoo
Copy link
Member

commented Sep 22, 2017

GIEs have a number of connectivity problems over less-than-perfect connections mainly due to two issues:

  1. Very short timeouts on the availability and keepalive requests
  2. The keepalive requests don't reset the failure count to 0 when successful

There were previously 3 different functions doing the same thing in different ways, these were refactored into a single function that will both increase the AJAX timeout as well as the retry (sleep) interval as desired.

With these changes, I'm able to start and maintain a Jupyter session with my browser set to simulate the speed of a 56k dialup connection...

@natefoo natefoo added this to the 17.09 milestone Sep 22, 2017

function make_spin_state(type, ajax_timeout_init, ajax_timeout_max, ajax_timeout_step, sleep_init, sleep_max, sleep_step, log_attempts){
var s = {};
s.type = (typeof type !== 'undefined') ? type : "GIE spin";
s.ajax_timeout = (typeof ajax_timeout_init !== 'undefined') ? ajax_timeout_init : 2000;

This comment has been minimized.

Copy link
@erasche

erasche Sep 23, 2017

Member

not a JS expert but I feel like i've usually seen this written as:

var s = {
  ajax_timeout: ajax_timeout_init || 2000
  ...
}

This comment has been minimized.

Copy link
@anuprulez

anuprulez Sep 23, 2017

Member

I agree with @erasche.
It could also be written as:
s.ajax_timeout = ( ajax_timeout_init ) ? ajax_timeout_init : 2000
These statements can check for null, false, NaN etc values as well unless there is a specific need to check only for "undefined".

This comment has been minimized.

Copy link
@natefoo

natefoo Sep 23, 2017

Author Member

Thanks! I am about as far from a JS expert as you can get, so I appreciate the guidance. I'll fix it in the next commit.

This comment has been minimized.

Copy link
@dannon

dannon Sep 23, 2017

Member

Just be careful when doing that. Personally I prefer the explicit checks. Using compound truth-test logic like this is what caused the missing history bug that took forever to find.

This comment has been minimized.

Copy link
@natefoo

natefoo Sep 26, 2017

Author Member

Well, you make a good point @dannon. What with JS's weak typing and wacky implicit type conversions, maybe the explicit check here is more error-proof?

This comment has been minimized.

Copy link
@dannon

dannon Sep 26, 2017

Member

It is definitely more error proof. I do prefer one part of @erasche's suggestion, though. I would write this more like:

var s = {
  ajax_timeout: (typeof ajax_timeout_init !== 'undefined') ? ajax_timeout_init : 2000;
  ...
}

This comment has been minimized.

Copy link
@dannon

dannon Sep 26, 2017

Member

(that said, I'm totally ok with leaving this exactly as you first wrote it, and wouldn't bother to rewrite it until after babel is available, which will allow for much better changes like default arguments to functions)

This comment has been minimized.

Copy link
@martenson

martenson Sep 26, 2017

Member

I am also for explicit here, this has been identified as critical Galaxy problem so let's not leave any room for ambiguity.

@natefoo natefoo changed the title [WIP] Client side fixes for GIEs Client side fixes for GIEs Sep 26, 2017

@natefoo natefoo added status/review and removed status/WIP labels Sep 26, 2017

@natefoo

This comment has been minimized.

Copy link
Member Author

commented Sep 26, 2017

I'd appreciate if anyone other than me tested these changes and played around with various throttling values (in developer tools) in each stage or other failure modes (killing Galaxy during each stage), etc., to see if they can induce any kind of catastrophic failures.

@natefoo

This comment has been minimized.

Copy link
Member Author

commented Sep 26, 2017

Well, huh, turns out that if the container is up but the notebook isn't listening, the failure is not a timeout, but a 502, so I'll make another change to account for that.

@jmchilton

This comment has been minimized.

Copy link
Member

commented Sep 26, 2017

@natefoo I don't think you should just replace Toastr like this. I think you should have to learn how to use a new package manager first - see https://github.com/galaxyproject/galaxy/blob/dev/client/bower.json#L19.

@dannon

This comment has been minimized.

Copy link
Member

commented Sep 26, 2017

I wouldn't bother to learn bower right now. It's dead and gone in the es6 branch (well, almost, but it will be completely eradicated soon), not to mention the greater javascript ecosystem.

@dannon

This comment has been minimized.

Copy link
Member

commented Sep 26, 2017

@natefoo I'll try to get GIEs set up and running locally to test this for ya tonight.

@natefoo

This comment has been minimized.

Copy link
Member Author

commented Sep 26, 2017

@dannon Thanks! They work out of the box for me using the default proxy configuration (automatically started by Galaxy on demand) as long as I npm install in the js proxy directory as per the documentation.

@natefoo

This comment has been minimized.

Copy link
Member Author

commented Sep 26, 2017

@jmchilton Well FWIW the case I needed the update for, I'm no longer using, so I guess I can restore the original version.

natefoo added some commits Sep 26, 2017

Treat timeouts and errors the same, except that timeouts step the
ajax timeout of the next request. Also fixes for toastr messages.

@natefoo natefoo force-pushed the natefoo:gie-client branch from 04dfca6 to 180bfe7 Sep 26, 2017

@natefoo natefoo changed the title Client side fixes for GIEs [17.09] Client side fixes for GIEs Sep 26, 2017

@dannon

This comment has been minimized.

Copy link
Member

commented Sep 27, 2017

With natefoo#7, this works pretty well for me. Without it, IEs consistently die shortly after launch when left alone (there's no keepalive).

(updated it, may have been cache on my end but now this is working at a variety of throttled speeds with only the client rebuild)

@dannon

This comment has been minimized.

Copy link
Member

commented Sep 27, 2017

And, using an externally configured proxy, it does survive galaxy restarts. Using the dynamic proxy the socket is lost when Galaxy dies, as expected (at least, I expected it).

Merge pull request #7 from dannon/gie_fixes
Re-enable keepalive, build client.
@natefoo

This comment has been minimized.

Copy link
Member Author

commented Sep 27, 2017

Thanks @dannon!

@dannon dannon merged commit 8230d14 into galaxyproject:release_17.09 Sep 27, 2017

1 of 6 checks passed

api test Test started.
Details
framework test Test started.
Details
integration test Test started.
Details
lgtm analysis: JavaScript Running analyses for revisions
Details
toolshed test Test started.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dannon

This comment has been minimized.

Copy link
Member

commented Sep 27, 2017

Sure. Way more robust now!

(merged forward to -dev)

@natefoo natefoo referenced this pull request Sep 27, 2017

Open

GIE Hardening #4651

9 of 21 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.