Skip to content

Begin to use std::mutex in place of ArchMutex - #8674

Merged
nbolton merged 10 commits into
masterfrom
barrier410
Jun 14, 2025
Merged

Begin to use std::mutex in place of ArchMutex#8674
nbolton merged 10 commits into
masterfrom
barrier410

Conversation

@sithlord48

@sithlord48 sithlord48 commented Jun 10, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Begin to replace ArchMutex with std::mutex. In this first step all cases of ArchMutex or Mutex that were not part of a conditional var have been replaced.

Tested locally for a few days didn't see any issues.

Backport from debauchee#410:

  • use std::mutex in arch/unix/ArchMutithreadPosix
  • use std::mutex in arch/ArchStrings
  • use std::mutex in base/Log
  • use std::mutex in base/EventQueue

Additionally:

  • Links arch to base
  • use std::mutex in arch/win32/ArchMultiThreadWindows
  • use std::mutex in arch/unix/ArchNetworkBSD
  • use std::mutex in arch/unix/ArchNetworkWinSock
  • use std::mutex in deskflow/PacketStreamFilter
  • use std::mutex in net/TCPListenSocket
  • use std::mutex in platform/XWindowsEventQueue

@sithlord48
sithlord48 force-pushed the barrier410 branch 2 times, most recently from 4db2217 to a3f4666 Compare June 10, 2025 23:59
@sithlord48 sithlord48 changed the title backport: https://github.com/debauchee/barrier/pull/410 Use std::mutex Jun 11, 2025
@sithlord48 sithlord48 changed the title Use std::mutex Backport: (barrier#410) begin to use std::mutex in place of ArchMutex Jun 11, 2025
@sithlord48 sithlord48 changed the title Backport: (barrier#410) begin to use std::mutex in place of ArchMutex Begin to use std::mutex in place of ArchMutex Jun 11, 2025
@sithlord48
sithlord48 marked this pull request as ready for review June 11, 2025 01:51
@sithlord48
sithlord48 requested a review from nbolton June 11, 2025 02:14
@sithlord48
sithlord48 force-pushed the barrier410 branch 2 times, most recently from 9402090 to ef8815a Compare June 12, 2025 03:27
@sithlord48 sithlord48 added this to the v1.23.0 milestone Jun 12, 2025
@nbolton
nbolton force-pushed the barrier410 branch 2 times, most recently from e6d48c9 to bdc2645 Compare June 14, 2025 08:09
@nbolton
nbolton enabled auto-merge (rebase) June 14, 2025 08:29
@nbolton
nbolton disabled auto-merge June 14, 2025 08:37
@nbolton
nbolton merged commit 782702f into master Jun 14, 2025
@nbolton
nbolton deleted the barrier410 branch June 14, 2025 08:37
jwakely
jwakely reviewed Sep 4, 2025
View reviewed changes
Comment thread src/lib/arch/ArchString.cpp
#include <mutex>

static ArchMutex s_mutex = nullptr;
std::mutex s_mutex;

@jwakely jwakely Sep 4, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this still be static, or in an anonymous namespace? It doesn't need to be globally visible (and possibly clash with an s_mutex in other translation units)

Comment thread src/lib/arch/unix/ArchNetworkBSD.cpp
Comment on lines +602 to +605
{
std::lock_guard<std::mutex> lock(m_mutex);
inet_ntop(AF_INET6, &ipAddr->sin6_addr, strAddr, INET6_ADDRSTRLEN);
}

@jwakely jwakely Sep 4, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I assume the purpose of the extra braces is so that the mutex is unlocked before constructing the std::string return value from the array strAddr? In that case, why is the kINIT case above different? That one constructs the string s (and move constructs it into the return value) while the mutex is held.

@sithlord48 sithlord48 Sep 4, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Honestly not sure this was ported from downstream i would think they should both have braces.

Step 1 was to Port, Now we can improve it feel free to propose a PR

@jwakely jwakely Sep 4, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Submitted as #8922

jwakely added a commit to jwakely/deskflow that referenced this pull request Sep 4, 2025
@jwakely
Give internal linkage to global mutex
cffbde2 
The `static` keyword was removed by deskflow#8674 but I think it should be restored so that this global can't clash with anything else called `::s_mutex` elsewhere in the executable.
@jwakely jwakely mentioned this pull request Sep 4, 2025
Merged
jwakely added a commit to jwakely/deskflow that referenced this pull request Sep 4, 2025
@jwakely
refactor: Give internal linkage to global mutex
13f0407 
The `static` keyword was removed by deskflow#8674 but I think it should be
restored so that this global can't clash with anything else called
`::s_mutex` elsewhere in the executable.
nbolton pushed a commit that referenced this pull request Sep 4, 2025
@jwakely @nbolton
refactor: Give internal linkage to global mutex
b788d63 
The `static` keyword was removed by #8674 but I think it should be
restored so that this global can't clash with anything else called
`::s_mutex` elsewhere in the executable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nbolton nbolton nbolton approved these changes
+1 more reviewer
@jwakely jwakely jwakely left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v1.23.0

Development

Successfully merging this pull request may close these issues.

4 participants

@sithlord48 @nbolton @jwakely @p12tic