Default timeout setting helper in process module does not work as intended #345
Labels
Comments
lukpueh
added a commit
to lukpueh/securesystemslib
that referenced
this issue
Feb 9, 2023
Prior to this commit, settings.SUBPROCESS_TIMEOUT=3 (seconds) was used as default and also to configure timeouts for any subprocess spawned via the securesystemslib process interface, including running the gpg command to sign, export keys and check version. The small timeout regularly lead to test failures on slow test runners. Furthermore, configuring the timeout via global is bad practice and error-prone (see secure-systems-lab#219, secure-systems-lab#345). This patch defines a custom constant, to be used only for gpg subprocesses and increases its value to 10 seconds. To override this default, an optional timeout argument will be added to relevant gpg interfaces in a subsequent commit. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
lukpueh
added a commit
to lukpueh/securesystemslib
that referenced
this issue
Feb 9, 2023
Prior to this change, configuring timeouts for gpg required patching the default-specifying global. This is bad practice and error-prone (see secure-systems-lab#219, secure-systems-lab#345). To allow overriding default timeout in an intuitive way, this commit adds optional arguments to the relevant gpg interface functions. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
lukpueh
added a commit
to lukpueh/securesystemslib
that referenced
this issue
Feb 9, 2023
Prior to this commit, settings.SUBPROCESS_TIMEOUT=3 (seconds) was used as default and also to configure timeouts for any subprocess spawned via the securesystemslib process interface, including running the gpg command to sign, export keys and check version. The small timeout regularly lead to test failures on slow test runners. Furthermore, configuring the timeout via global is bad practice and error-prone (see secure-systems-lab#219, secure-systems-lab#345). This patch defines a custom constant, to be used only for gpg subprocesses and increases its value to 10 seconds. To override this default, an optional timeout argument will be added to relevant gpg interfaces in a subsequent commit. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
lukpueh
added a commit
to lukpueh/securesystemslib
that referenced
this issue
Feb 9, 2023
Prior to this change, configuring timeouts for gpg required patching the default-specifying global. This is bad practice and error-prone (see secure-systems-lab#219, secure-systems-lab#345). To allow overriding default timeout in an intuitive way, this commit adds optional arguments to the relevant gpg interface functions. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
lukpueh
added a commit
to lukpueh/securesystemslib
that referenced
this issue
Feb 10, 2023
Prior to this commit, settings.SUBPROCESS_TIMEOUT=3 (seconds) was used as default and also to configure timeouts for any subprocess spawned via the securesystemslib process interface, including running the gpg command to sign, export keys and check version. The small timeout regularly lead to test failures on slow test runners. Furthermore, configuring the timeout via global is bad practice and error-prone (see secure-systems-lab#219, secure-systems-lab#345). This patch defines a custom constant, to be used only for gpg subprocesses and increases its value to 10 seconds. To override this default, an optional timeout argument will be added to relevant gpg interfaces in a subsequent commit. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
lukpueh
added a commit
to lukpueh/securesystemslib
that referenced
this issue
Feb 10, 2023
Prior to this change, configuring timeouts for gpg required patching the default-specifying global. This is bad practice and error-prone (see secure-systems-lab#219, secure-systems-lab#345). To allow overriding default timeout in an intuitive way, this commit adds optional arguments to the relevant gpg interface functions. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
lukpueh
added a commit
to lukpueh/securesystemslib
that referenced
this issue
Feb 10, 2023
This module was originally developed in in-toto and transferred to securesystemslib in secure-systems-lab#174, primarily as Py2/Py3-agnostic wrapper around stdlib's `subprocess.run`, to to execute `gpg`. In secure-systems-lab#502 we switched to using `subprocess.run` directly. Another wrapper around `subprocess.run`, provided by the module, allows capturing standard streams but still write them to a terminal. It was developed as specific `in-toto-run` feature and does not need to be public API in sslib. in-toto/in-toto#544 moves the function back to in-toto. closes secure-systems-lab#345, transfers secure-systems-lab#337 (to in-toto) Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
lukpueh
added a commit
to lukpueh/securesystemslib
that referenced
this issue
Feb 10, 2023
This module was originally developed in in-toto and transferred to securesystemslib in secure-systems-lab#174, primarily as Py2/Py3-agnostic wrapper around stdlib's `subprocess.run`, to run the `gpg` command. In secure-systems-lab#502 we switched to using `subprocess.run` directly. Another wrapper around `subprocess.run`, provided by the module, allows capturing standard streams AND write them to a terminal. It was developed as specific `in-toto-run` feature and does not need to be public API in sslib. in-toto/in-toto#544 moves the function back to in-toto. closes secure-systems-lab#345, transfers secure-systems-lab#337 (to in-toto) Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Description of issue or feature request:
#202 added a helper function with the intention of deferring the evaluation of the default argument
SUBPROCESS_TIMEOUT, used in two functions of theprocessmodule, until invocation of these functions.This should have made it possible to modify
SUBPROCESS_TIMEOUTvia thesettingsmodule, where it is defined originally, even after importing theprocessmodule.Unfortunately, this does not work as intended, because the helper function and thus the default
SUBPROCESS_TIMEOUTis still evaluated only once at import time.Note: This issue is a side-effect of the non-intuitive usage pattern of expecting user interaction via patching globals in the
settingsmodule, which is documented in #219. IMHO it is okay to fix this issue together with #219.Current behavior:
Default argument is evaluated at function import time.
Expected behavior:
Default argument is evaluated at function invocation time.
The text was updated successfully, but these errors were encountered: