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

Fix the barrier logic for threads synchronization #811

Merged
merged 5 commits into from
Feb 20, 2024

Conversation

LoreMoretti
Copy link
Contributor

@LoreMoretti LoreMoretti commented Feb 16, 2024

As explained in this comment, we found out that the barrier logic for threads synchronization is not working properly.

This is due to the following two reasons:

  1. No thread will go to the else branch if the if condition reads ((--m_count) == 1). Therefore no thread will wait.
  2. The wait() function is not included in the lambda function passed to std::thread(). This means that, even if the wait command is reached, it will stop the main thread (i.e. the one which is spawning the other threads).

This PR aims at addressing these problems.

Copy link
Member

@GiulioRomualdi GiulioRomualdi left a comment

Choose a reason for hiding this comment

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

Well done! I've added some comments to improve the PR but overall it is fine! Thanks for spending time and fixing this 🎉

src/System/src/Barrier.cpp Outdated Show resolved Hide resolved
src/System/src/Barrier.cpp Outdated Show resolved Hide resolved
src/System/src/Barrier.cpp Show resolved Hide resolved
@LoreMoretti LoreMoretti reopened this Feb 19, 2024
@LoreMoretti
Copy link
Contributor Author

I have closed and re-opened the PR to trigger again the CI, because Windows jobs were failing (probably due to a connection error, since the download of the wget package was failing). But it seems like these Windows jobs are failing again for the same reason.

@GiulioRomualdi could you try to trigger again just the Windows jobs? I think I do not have the rights to trigger specific jobs. Thanks!

@traversaro
Copy link
Collaborator

For reference, the error is:

Chocolatey v2.2.2
Installing the following packages:
wget;unzip
By installing, you accept licenses for the packages.
Invalid credentials specified.
Please provide credentials for: https://community.chocolatey.org/api/v2/
User name: Password: Chocolatey had an error on fv-az1389-843 (with user runneradmin):
System.InvalidOperationException: Cannot read keys when either application does not have a console or when console input has been redirected from a file. Try Console.Read.
   at System.Console.ReadKey(Boolean intercept)
   at chocolatey.infrastructure.commandline.ReadKeyTimeout.ConsoleReadKey()
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state, Boolean preserveSyncCtx)
   at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state, Boolean preserveSyncCtx)
   at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state)
   at System.Threading.ThreadHelper.ThreadStart()

Unhandled Exception: System.InvalidOperationException: Cannot read keys when either application does not have a console or when console input has been redirected from a file. Try Console.Read.
   at System.Console.ReadKey(Boolean intercept)
   at chocolatey.infrastructure.commandline.ReadKeyTimeout.ConsoleReadKey()
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state, Boolean preserveSyncCtx)
   at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state, Boolean preserveSyncCtx)
   at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state)
   at System.Threading.ThreadHelper.ThreadStart()
wget: D:\a\_temp\6bc5d996-a0b3-49ae-adb6-33659e0889e6.ps1:7
Line |
   7 |  wget https://github.com/robotology/robotology-superbuild-dependencies …
     |  ~~~~
     | The term 'wget' is not recognized as a name of a cmdlet, function, script file, or executable program. Check the
     | spelling of the name, or if a path was included, verify that the path is correct and try again.
Error: Process completed with exit code 1.

Not sure if it is a network error. Perhaps we could use curl instead of wget to download the .zip on Windows?

@traversaro
Copy link
Collaborator

For reference, the error is:

Chocolatey v2.2.2
Installing the following packages:
wget;unzip
By installing, you accept licenses for the packages.
Invalid credentials specified.
Please provide credentials for: https://community.chocolatey.org/api/v2/
User name: Password: Chocolatey had an error on fv-az1389-843 (with user runneradmin):
System.InvalidOperationException: Cannot read keys when either application does not have a console or when console input has been redirected from a file. Try Console.Read.
   at System.Console.ReadKey(Boolean intercept)
   at chocolatey.infrastructure.commandline.ReadKeyTimeout.ConsoleReadKey()
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state, Boolean preserveSyncCtx)
   at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state, Boolean preserveSyncCtx)
   at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state)
   at System.Threading.ThreadHelper.ThreadStart()

Unhandled Exception: System.InvalidOperationException: Cannot read keys when either application does not have a console or when console input has been redirected from a file. Try Console.Read.
   at System.Console.ReadKey(Boolean intercept)
   at chocolatey.infrastructure.commandline.ReadKeyTimeout.ConsoleReadKey()
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state, Boolean preserveSyncCtx)
   at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state, Boolean preserveSyncCtx)
   at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state)
   at System.Threading.ThreadHelper.ThreadStart()
wget: D:\a\_temp\6bc5d996-a0b3-49ae-adb6-33659e0889e6.ps1:7
Line |
   7 |  wget https://github.com/robotology/robotology-superbuild-dependencies …
     |  ~~~~
     | The term 'wget' is not recognized as a name of a cmdlet, function, script file, or executable program. Check the
     | spelling of the name, or if a path was included, verify that the path is correct and try again.
Error: Process completed with exit code 1.

Not sure if it is a network error. Perhaps we could use curl instead of wget to download the .zip on Windows?

Related upstream issue: actions/runner-images#9367 (comment) . There still seems to be problem: https://status.chocolatey.org/ . Probably we can just ignore the Windows-vcpkg job for now, as long as Windows conda is compiling fine, I do not think we can risk regressions.

@traversaro
Copy link
Collaborator

I restarted the CI as Chocolatey is now up: https://status.chocolatey.org/ .

@GiulioRomualdi GiulioRomualdi merged commit eb4b82d into ami-iit:master Feb 20, 2024
22 of 24 checks passed
@GiulioRomualdi
Copy link
Member

Thank you @LoreMoretti

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants