Skip to content

Conversation

@LukaszMrugala
Copy link
Contributor

If you interrupt process() operation, we want Twister to exit as gracefully as it can. This avoids the UnboundLocalError that could appear e.g. when interrupting the operation via SIGINT.

Currently, we can experience a situation like this:

INFO    - Adding tasks to the queue...
INFO    - Added initial list of jobs to queue
^CERROR   - General exception: cannot access local variable 'next_op' where it is not associated with a value
ERROR   - General exception: cannot access local variable 'next_op' where it is not associated with a value
ERROR   - General exception: cannot access local variable 'next_op' where it is not associated with a value
ERROR   - General exception: cannot access local variable 'next_op' where it is not associated with a value
ERROR   - General exception: cannot access local variable 'next_op' where it is not associated with a value
ERROR   - General exception: cannot access local variable 'next_op' where it is not associated with a value
ERROR   - General exception: cannot access local variable 'next_op' where it is not associated with a value
ERROR   - General exception: cannot access local variable 'next_op' where it is not associated with a value
ERROR   - General exception: cannot access local variable 'next_op' where it is not associated with a value
ERROR   - General exception: cannot access local variable 'next_op' where it is not associated with a value
ERROR   - General exception: cannot access local variable 'next_op' where it is not associated with a value
ERROR   - General exception: cannot access local variable 'next_op' where it is not associated with a value
INFO    - Execution interrupted

@LukaszMrugala LukaszMrugala added bug The issue is a bug, or the PR is fixing a bug area: Twister Twister labels Sep 12, 2024
@LukaszMrugala LukaszMrugala marked this pull request as ready for review September 12, 2024 09:12
@golowanow
Copy link
Member

golowanow commented Sep 12, 2024

Although I can't always reproduce the issue with Ctrl-C at precise moment, it seems reasonable to set these initial values, but I'm not sure yet is it enough to address the problem and exit gracefully. For example, I've also got errors like this:

^CINFO    - Execution interrupted
...
ERROR   - some_test: Unknown status 'None'

@LukaszMrugala
Copy link
Contributor Author

LukaszMrugala commented Sep 12, 2024

^CINFO    - Execution interrupted
...
ERROR   - some_test: Unknown status 'None'

This would be caused by the interrupt making the instance get incorrect status and then proceeding to the report. Were you able to find what code section, when interrupted, results in that? It seems to be breaking the assumption of using TwisterStatus as status, so I'm interested in the mechanism of failure there.

At the same time, I'd point out that normally, I do not expect much from a CLI programme that I interrupt via Ctrl-C, save for it quitting.

@nashif
Copy link
Member

nashif commented Sep 12, 2024

^CINFO - Execution interrupted
...
ERROR - some_test: Unknown status 'None'

that is the original behavior and it is correct when you interrupt execution. Lets just fix this new bug and deal with improvement, if needed, later.

@LukaszMrugala
Copy link
Contributor Author

LukaszMrugala commented Sep 12, 2024

^CINFO - Execution interrupted
...
ERROR - some_test: Unknown status 'None'

that is the original behavior and it is correct when you interrupt execution. Lets just fix this new bug and deal with improvement, if needed, later.

I have taken a look at it and it is slightly incorrect. It should report "No status", not "Unknown status", as per code. We ideally should never have a status that is not a TwisterStatus inside Twister. This is caused by dict.get('status') not being parametrised correctly here. A quick search reveal that it occurs semi-frequently in the code and I will create a separate PR to fix that.

If you interrupt process() operation, we want Twister
to exit as gracefully as it can. This avoids the
UnboundLocalError that could appear e.g.
when interrupting the operation via SIGINT.

Signed-off-by: Lukasz Mrugala <lukaszx.mrugala@intel.com>
@LukaszMrugala LukaszMrugala force-pushed the lmrugalx_process_keyboardinterrupt_hardening branch from 6c2e0c7 to bb26c51 Compare September 12, 2024 10:19
Copy link
Member

@golowanow golowanow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.. Lets just fix this new bug and deal with improvement, if needed, later.

I agree that better to fix this noisy bug, but I still suspect there might be other side effects from the recent design decision #77080 to put per-operation StatusAttributeError handlers with intent to continue execution there.

@nashif nashif added the Trivial Changes that can be reviewed by anyone, i.e. doc changes, minor build system tweaks, etc. label Sep 13, 2024
@carlescufi carlescufi merged commit f78949b into zephyrproject-rtos:main Sep 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Twister Twister bug The issue is a bug, or the PR is fixing a bug Trivial Changes that can be reviewed by anyone, i.e. doc changes, minor build system tweaks, etc.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants