requests calls always need a timeout definition#2727
requests calls always need a timeout definition#2727ReimarBauer merged 9 commits intoOpen-MSS:stablefrom
Conversation
mslib/msui/mscolab_chat.py
Outdated
| else: | ||
| img_url = urljoin(self.chat_window.mscolab_server_url, self.attachment_path) | ||
| data = requests.get(img_url, timeout=tuple(config_loader(dataset="MSCOLAB_timeout"))).content | ||
| data = requests.get(img_url, timeout=MSCOLAB_TIMEOUT).content |
There was a problem hiding this comment.
Unrelated issue: this accesses the responses content without first checking for success.
There was a problem hiding this comment.
yes, first step I wanted to solve only timeout configurations. There are likly more like that.
mslib/utils/mssautoplot.py
Outdated
| session.headers.update({'x-test': 'true'}) | ||
| # ToDp fix config_loader it gets a list of two times the entry | ||
| response = session.get(urljoin(msc_url, 'status'), timeout=tuple(config_loader(dataset="MSCOLAB_timeout")[0])) | ||
| response = session.get(urljoin(msc_url, 'status'), timeout=MSCOLAB_TIMEOUT[0]) |
There was a problem hiding this comment.
Why does it only take the first element here, unlike in the other places?
There was a problem hiding this comment.
I have no idea, I had that once changed to the common, but decided to keep the value until that question is looked up.
There was a problem hiding this comment.
This is a bug #2730 the ToDo was a hint.
I try to write a test for that first.
mslib/msui/mscolab.py
Outdated
| from mslib.utils.config import config_loader, modify_config_file | ||
|
|
||
|
|
||
| MSCOLAB_TIMEOUT = tuple(config_loader(dataset="MSCOLAB_timeout")) |
There was a problem hiding this comment.
Does this actually make a measurable difference?
There was a problem hiding this comment.
less characters on other places and the option to try different values for a module.
There was a problem hiding this comment.
The downside is that this could lead to using an outdated value when the configuration is changed while the application is running. I think this value wouldn't be updated if MSCOLAB_timeout is changed in the configuration editor, even with the application restart that it does.
There was a problem hiding this comment.
the editor has another bug -> new issue coming.
and there is another problem by that var or on the MBP
That was not expected.
There was a problem hiding this comment.
Anyway it is more important to have always a timeout there then a short cut module global. It is an easy refactoring.
matrss
left a comment
matrss
left a comment
There was a problem hiding this comment.
There are two locations in docs/samples/automation/retriever.py where there is no timeout specified yet.
That was removed because it is outdated |
2 participants
Purpose of PR?:
... or they can indefinite wait
Also set per module a global MSCOLAB_TIMEOUT to reduce calls of config_loader
Fixes #2726