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
[][SVCS-683] Add timeouts for subprocess calls #332
[][SVCS-683] Add timeouts for subprocess calls #332
Conversation
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.
Here is a few minor issues and back to you 🔥 :
- Rebase to the latest
develop
and fix travis. - Update style.
- Since
unoconv
already have the fix, can you provide a link so we know if this is a similar one or not? Are the 10 seconds what we've agreed on or just a experimental number? Is this code eligible for test? It would be great if you can provide more information on the PR.
mfr/extensions/jsc3d/export.py
Outdated
self.output_file_path, | ||
# silence output from freecadcmd | ||
], stdout=subprocess.DEVNULL) | ||
subprocess.check_call( |
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.
How about the following style?
cmd = [FREECAD_BIN, FREECAD_CONVERT_SCRIPT, temp_source_file.name, self.output_file_path]
subprocess.check_call(cmd, stdout=subprocess.DEVNULL, timeout=settings.FREECAD_TIMEOUT)
mfr/extensions/jsc3d/export.py
Outdated
FREECAD_CONVERT_SCRIPT, | ||
temp_source_file.name, | ||
self.output_file_path, | ||
# silence output from freecadcmd |
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.
Move this misplaced comments to the right place.
mfr/extensions/jsc3d/export.py
Outdated
code=504, | ||
process='freecad', | ||
cmd=str(err.cmd), | ||
returncode=52, # This is not re error code returned by the |
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 commenting style is far from Pythonic. You can move it to before the raise
as a comment for the raise
statement.
mfr/extensions/tabular/utilities.py
Outdated
fp.name, | ||
csv_file.name, | ||
]) | ||
subprocess.check_call( |
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.
Similar style update as mentioned before.
mfr/extensions/tabular/utilities.py
Outdated
code=504, | ||
process='pspp', | ||
cmd=str(err.cmd), | ||
returncode=52, # This is not re error code returned by the |
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.
Similar style update as mentioned before.
fecdf33
to
4e0a287
Compare
mfr/extensions/default/exporter.py
Outdated
@@ -0,0 +1,5 @@ | |||
from mfr.core.extension import BaseExporter | |||
|
|||
class DefaultExporter(BaseExporter): |
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.
get rid of me
Needs a rebase |
4e0a287
to
58fe0d6
Compare
3b94e88
to
00096d5
Compare
3f230c2
to
81b5b8c
Compare
81b5b8c
to
2862304
Compare
All three of the current exporters use a subprocess call to perform the export. To prevent the process from hanging, we add a condition to kill th process after a specified period of time.
2862304
to
05408e9
Compare
Ticket
SVCS-683 Add timeouts for subprocess calls
Purpose
Make subprocess time out instead of hanging indefinitely.
Changes
Side effects
May make files that are particularly large time out if they take too long.
QA Notes
Deployment Notes
Adds a couple of ENV vars to configure the timeout if desired. Default set to 10s.