-
Notifications
You must be signed in to change notification settings - Fork 312
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
Add StellarTomlResolver timeout option #166
Conversation
It looks good however it won't work if stellar.toml file is requested in federation context (
You are correct. You can start a simple HTTP server like here with a |
Made the following changes:
|
I also see that this
However, unless I am mistaken, it looks like the |
…TomlResolver tests
src/config.js
Outdated
* Returns the value of `timeout` flag. | ||
* @static | ||
*/ | ||
static hasTimeout() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be getTimeout
.
it("rejects after given timeout when global Config.timeout flag is set", function (done) { | ||
StellarSdk.Config.setTimeout(1000); | ||
|
||
StellarSdk.StellarTomlResolver.resolve('acme.com') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests shouldn't make network connections (other to local test server).
I think it's good to have a global and local settings. For example you may want to change the We should also add |
Fixed the above:
Also looked at where the Besides
So I added it to |
…d up a few things
I all looks good, but I'd change one last thing: we should probably add
We shouldn't change |
Following my last commit that addressed your feedback, I have a few questions:
|
… add timeout to call_builder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I include options (such as allowHttp) even in places where it won't be used (such as in some of the federation_server methods)?
I added some inline comments. allowHttp
is class setting checked in constructor (for attempt to use http server if it's not allowed the object won't be created). So I think we don't need other options than timeout
for class methods.
For resolveAddress, resolveAccountId, resolveTransactionId in the federation_server is it best to pass in opts like I do here?
I think we should only allow setting timeout
for static
methods. For normal methods the value should be taken from this.timeout
set in constructor.
For call_builder should I pass in timeout as an opts param like I do or should I just pass in timeout?
Let's leave opts
, will be helpful for other options in a future.
Can we also add some tests for a) static methods, b) normal methods on FederationServer
, c) methods using CallBuilder
s on 'Server'.
src/call_builder.js
Outdated
* @class CallBuilder | ||
*/ | ||
export class CallBuilder { | ||
constructor(serverUrl) { | ||
constructor(serverUrl, opts = {}) { | ||
this.timeout = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default here should be a global setting so Config.getTimeout()
.
src/config.js
Outdated
@@ -32,6 +35,16 @@ class Config { | |||
config.allowHttp = value; | |||
} | |||
|
|||
/** | |||
* Sets `timeout` flag globally. When set to anything besides 0, StellarTOMLResolver will timeout after specified time (ms). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update the comment. It's not StellarTOMLResolver
only anymore.
src/federation_server.js
Outdated
@@ -20,6 +20,7 @@ export const FEDERATION_RESPONSE_MAX_SIZE = 100 * 1024; | |||
* @param {string} domain Domain this server represents | |||
* @param {object} [opts] | |||
* @param {boolean} [opts.allowHttp] - Allow connecting to http servers, default: `false`. This must be set to false in production deployments! You can also use {@link Config} class to set this globally. | |||
* @param {number} [opts.timeout] - Allow a timeout, default: 0. In ms. | |||
*/ | |||
export class FederationServer { | |||
constructor(serverURL, domain, opts = {}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Static method instantiate this class but opts.timeout
is not persisted anywhere.
src/federation_server.js
Outdated
|
||
if (typeof opts.allowHttp !== 'undefined') { | ||
allowHttp = opts.allowHttp; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for allowHttp
processing here, it's been checked in constructor
.
src/federation_server.js
Outdated
} | ||
|
||
/** | ||
* Returns a Promise that resolves to federation record if the sender of the transaction was found for a given transaction ID. | ||
* @see <a href="https://www.stellar.org/developers/learn/concepts/federation.html" target="_blank">Federation doc</a> | ||
* @param {string} transactionId Transaction ID (ex. `3389e9f0f1a65f19736cacf544c2e825313e8447f569233bb8db39aa607c8889`) | ||
* @param {object} [opts] | ||
* @param {boolean} [opts.allowHttp] - Allow connecting to http servers, default: `false`. This must be set to false in production deployments! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
allowHttp
is not needed here as it's checked in constructor
per server.
src/server.js
Outdated
@@ -32,6 +32,7 @@ export const SUBMIT_TRANSACTION_TIMEOUT = 60*1000; | |||
* @param {string} serverURL Horizon Server URL (ex. `https://horizon-testnet.stellar.org`). | |||
* @param {object} [opts] | |||
* @param {boolean} [opts.allowHttp] - Allow connecting to http servers, default: `false`. This must be set to false in production deployments! You can also use {@link Config} class to set this globally. | |||
* @param {number} [opts.timeout] - Allow a timeout, default: 0. In ms. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not passed to CallBuilder
anywhere.
@bartekn started adding Since that code started to get a bit long and since this PR is focused on solving #158 which only deals with So what I now have is just the code for Is this reasoning sound? I think adding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I agree to limit this PR to StellarTomlResolver
and FederationServer
. There are still a few issues to fix.
src/federation_server.js
Outdated
@@ -161,7 +163,9 @@ export class FederationServer { | |||
} | |||
|
|||
_sendRequest(url) { | |||
return axios.get(url.toString(), {maxContentLength: FEDERATION_RESPONSE_MAX_SIZE}) | |||
let timeout = this.timeout | Config.getTimeout(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.timeout
is not set anywhere.
In general it should work like this:
- In
constructor
setthis.timeout
toopts.timeout
orConfig.getTimeout()
(if former not set). static
method createFederationServer
instance internally so we should just passopts
to class constructor.- Finally
_sendRequest
should just usethis.timeout
. No need to checkConfig
again.
Also, should be ||
. |
is bitwise OR.
test/unit/federation_server_test.js
Outdated
|
||
describe('FederationServer times out when response lags and timeout set', function () { | ||
it("resolveAddress times out", function (done) { | ||
StellarSdk.Config.setTimeout(100); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should reset it to default value so it doesn't influence other tests. Maybe in afterEach
? Would be great to have We already have Config.setDefaults()
or something like this, in case default values change in a future.Config.setDefault()
.
test/unit/federation_server_test.js
Outdated
setTimeout(() => {}, 10000); | ||
}).listen(4444, () => { | ||
StellarSdk.FederationServer | ||
.resolve('bob*localhost:4444', {allowHttp: true, timeout: 100}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test shouldn't work because opts
is not passed to FederationServer.constructor
(see other comment). It only works because StellarSdk.Config.setTimeout(100);
is not reset after previous tests.
test/unit/federation_server_test.js
Outdated
let tempServer = http.createServer((req, res) => { | ||
setTimeout(() => {}, 10000); | ||
}).listen(4444, () => { | ||
new StellarSdk.FederationServer('http://localhost:4444/federation', 'stellar.org', {allowHttp: true}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The last missing test case is creating a new FederationServer
with opts.timeout
set (without calling Config.setTimeout
).
I think you can create a loop that will test all the methods (resolveTransactionId
, resolveAccountId
, ...) with both cases: Config
in the first iteration and opts
in the second. Without it we will have a lot of code duplication here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than my last 2 comments looks great!
test/unit/federation_server_test.js
Outdated
// Unable to create temp server in a browser | ||
if (typeof window != 'undefined') { | ||
return done(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has to be checked inside it
s functions. done
is not defined here. You can run gulp test:browser
to test this locally. CI tests pass because we do not perform SauceLabs tests in PRs at it could leak our keys.
https://docs.travis-ci.com/user/environment-variables/#Defining-encrypted-variables-in-.travis.yml
Encrypted environment variables are not available to pull requests from forks due to the security risk of exposing such information to unknown code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, ok had to install firefox for this one
@@ -106,5 +106,42 @@ FEDERATION_SERVER="https://api.stellar.org/federation" | |||
.then(() => tempServer.close()); | |||
}); | |||
}); | |||
|
|||
it("rejects after given timeout when global Config.timeout flag is set", function (done) { | |||
StellarSdk.Config.setTimeout(1000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add Config.setDefault();
to afterEach
here too.
Closes: #158
Motivation
Say you are a service that wants to query all asset issuers and are using
StellarSdk.StellarTomlResolver.resolve(uri.host())
to get the TOML information. Unfortunately this sometimes gets caught up and can lag for long periods of time.An example url that causes this lag:
xggtoken.com
Solution
Add an optional timeout field to the opts param in
StellarSdk.StellarTomlResolver.resolve
.Using the timeout config in the axios fetch, we can let sdk users specify a timeout length (in
ms
) to avoid these long TOML resolve lags.In case users don't want to use the
timeout
config, it is set to 0 by default.Example usage:
await StellarTomlResolver.resolve("xggtoken.com", { timeout: 1000 });
Note
I thought about writing a test for this, however to future proof the test suite, I did not want to test against the example I have above in case this domain changes or is taken down. Is this reasoning correct?