Skip to content
This repository has been archived by the owner on Mar 27, 2019. It is now read-only.

[enhance] add request timeout on aixos #61

Closed
wants to merge 3 commits into from
Closed

[enhance] add request timeout on aixos #61

wants to merge 3 commits into from

Conversation

wgx731
Copy link
Contributor

@wgx731 wgx731 commented Feb 24, 2017

Hi guys,

I have noticed if Vault is not started or down, vault-ui will send request and seems wait forever. Therefore I think we should add request time out for axios module, but as we have two part (express server and react component) both using axios, we may need to find a clean way to do this right. Currently, time is only added to express server part.

NOTE: please check this commit message for TODOs.

Thank you very much.

1. load time out from env and default to 2000 ms
2. refactor login method to VaultUtils
3. add Consts helper
1. move timeout to express server part
2. change default timeout vaule
TODO:
1. fire only one request, cancel previous request if any
2. find a way to pass env to react component
3. ideally react part aixos should have time out as well
@msessa
Copy link
Collaborator

msessa commented Feb 27, 2017

Hi @wgx731,

I agree we should have a way to control timeouts but your PR seems to be putting most of the logic in the express side of the app.
Vault-UI is by all means a fully client side application, and the nodejs backend part is purely a dumb proxy passthrough.

This means all logic, even request timeouts, should be controlled by the client.

Please take a look at this branch I just pushed for a possible implementation
https://github.com/djenriquez/vault-ui/compare/request_timeout

@wgx731
Copy link
Contributor Author

wgx731 commented Feb 27, 2017

@msessa-cotd,

Yes, I agreed. In the first commit, I was trying to put the timeout only in client side, but I am facing problem accessing environment variable in react app. That's why I switched to add the logic in express server part.

If you already have a solution to this enhancement, then I will close this pull request.

Thank you very much.

@wgx731 wgx731 closed this Feb 27, 2017
@msessa
Copy link
Collaborator

msessa commented Feb 27, 2017

Thanks for bringing this issue to my attention.

I'll probably push this enhancement shortly after #60 is merged, since that PR removes the custom axios call on login in favour of the unified callVaultApi.

@wgx731 wgx731 deleted the request-timeout branch February 27, 2017 02:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants