Skip to content
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

Add CLS option to control infinite loop timeout #538

Merged
merged 1 commit into from
Nov 4, 2024

Conversation

furuame
Copy link
Member

@furuame furuame commented Oct 25, 2024

No description provided.

@furuame furuame added the component: sparta Issue is related to sparta framework label Oct 25, 2024
@furuame furuame self-assigned this Oct 25, 2024
@klingaard
Copy link
Member

Interesting addition. Curious on the use case to increase the timeout since I believe it defaults to a minute. I've never seen the timeout fire on simulations I've made. However, if you're integrating your simulator with another framework, I suggest disabling the thread instead of increasing the timeout.

Regardless, allowing control of the timeout period seems reasonable, but does it have to be a Sparta command line option?

Copy link
Member

@klingaard klingaard left a comment

Choose a reason for hiding this comment

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

I don't mind moving forward with this addition, but I'm still curious at the problem that prompted this addition. 😉

@furuame
Copy link
Member Author

furuame commented Oct 30, 2024

Interesting addition. Curious on the use case to increase the timeout since I believe it defaults to a minute. I've never seen the timeout fire on simulations I've made. However, if you're integrating your simulator with another framework, I suggest disabling the thread instead of increasing the timeout.

Yes exactly that scenario, basically there are two simulators in my scenario 🙃 One of them executed for a while and not moves forward. I checked and that works fine but just needs time. However, the other one thought it didn't make forward progress, so it broke. I still like to keep the SleeperThread just in case, it is still useful to find real bugs.

Regardless, allowing control of the timeout period seems reasonable, but does it have to be a Sparta command line option?

Since SleeperThread is a static class object, my currently deployed workaround is: simply call sparta::SleeperThread::getInstance()->setInfLoopSleepInterval at simulator initialization. My initial thought is: I should pass the parameter from my simulator to CLS, and let CLS handles it. What do you think?

@klingaard
Copy link
Member

Since SleeperThread is a static class object, my currently deployed workaround is: simply call sparta::SleeperThread::getInstance()->setInfLoopSleepInterval at simulator initialization. My initial thought is: I should pass the parameter from my simulator to CLS, and let CLS handles it. What do you think?

I think it's fine, I just see it as very rare that anyone would use it. But no bother to me if you want to merge this.

@furuame furuame merged commit a944066 into master Nov 4, 2024
8 checks passed
@furuame furuame deleted the zhenw/add-inf-loop-timeout-opt branch November 4, 2024 01:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: sparta Issue is related to sparta framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants