Skip to content

Bhamehta/code cleanup - #2

Open
bmehta001 wants to merge 26 commits into
mainfrom
bhamehta/code-cleanup
Open

Bhamehta/code cleanup#2
bmehta001 wants to merge 26 commits into
mainfrom
bhamehta/code-cleanup

Conversation

@bmehta001

@bmehta001 bmehta001 commented Mar 18, 2026

Copy link
Copy Markdown
Owner

Fixes runtime thread-safety, shutdown-safety, and response-lifetime issues that affect normal SDK operation. Split out from microsoft#1415 per reviewer request so runtime behavior changes stay separate from CI/build/test fixes.

Fix HTTP cancellation and callback tracking

HttpClient_Apple.mm

  • Scope cancellation to the current request’s m_dataTask instead of canceling every task on the shared static NSURLSession.
  • Fix the CancelAllRequests() wait loop so m_requests.empty() is checked while holding m_requestsMtx.

HttpClientManager.cpp

  • Fix the cancelAllRequests() wait loop so m_httpCallbacks.empty() is checked while holding m_httpCallbacksMtx.

Fix HTTP response lifetime on abort/network-failure paths

HttpResponseDecoder.cpp

  • Preserve ctx->httpResponse through requestAborted(ctx) and temporaryNetworkFailure(ctx) so downstream storage/statistics handlers can still read status and headers.
  • Avoid leaking aborted/network-failure responses by keeping ownership with EventsUploadContext, whose clear() path deletes the response.

HttpResponseDecoderTests.cpp

  • Add regression coverage that aborted and network-failure decode routes still receive the response object with result/status intact.

Fix WorkerThread shutdown safety

WorkerThread.cpp

  • Add an explicit shutdown gate so late Queue() calls are rejected once teardown starts.
  • Enqueue the shutdown sentinel only once under the queue lock.
  • Clean up pending queued/timer tasks only after a successful join(), and avoid cleanup after detach() because the worker may still be running.
  • Log join/detach failures instead of silently swallowing all exceptions.

Make TransmissionPolicyManager scheduling consistently mutex-guarded

TransmissionPolicyManager.cpp / .hpp

  • Guard m_isUploadScheduled, m_runningLatency, and m_scheduledUploadTime consistently with m_scheduledUploadMutex.
  • Avoid holding m_scheduledUploadMutex across potentially blocking cancellation during stop/shutdown.
  • Keep force/zero-delay no-wait cancellation under the scheduler mutex so a competing delayed schedule cannot suppress an immediate upload.
  • Use an explicit std::chrono::milliseconds value in the bandwidth-controller reschedule path.

TransmissionPolicyManagerTests.cpp

  • Add regression coverage for the force/zero-delay scheduling race.

Fix Logger static-destruction-order crash

Logger.cpp

  • Remove destructor logging from Logger::~Logger() because it can run after logging infrastructure has already been destroyed, causing crashes during static teardown.

Validation

  • HttpResponseDecoderTests.* and TransmissionPolicyManagerTests.ForceScheduleRetainsImmediateUploadWhenCancelBlocks passed.
  • Full host UnitTests passed: 485/485

@bmehta001 bmehta001 self-assigned this Mar 18, 2026
@bmehta001
bmehta001 force-pushed the bhamehta/code-cleanup branch 20 times, most recently from 9a5a166 to 2feeb94 Compare March 19, 2026 05:40
@bmehta001
bmehta001 force-pushed the bhamehta/code-cleanup branch 4 times, most recently from e303041 to 9a91839 Compare April 1, 2026 16:55
@bmehta001
bmehta001 force-pushed the bhamehta/code-cleanup branch 5 times, most recently from b30c9b8 to 95502d5 Compare April 29, 2026 21:29
@bmehta001
bmehta001 force-pushed the bhamehta/code-cleanup branch from b34147f to 0e8018c Compare May 15, 2026 03:06
@bmehta001
bmehta001 force-pushed the bhamehta/code-cleanup branch from 17b72ca to 1d09c10 Compare May 22, 2026 07:35
bmehta001 and others added 10 commits May 22, 2026 15:02
- Modernize CMakeLists.txt: flatten nesting, deduplicate, use
  consistent quoting and variable patterns
- Remove stale header references from vcxproj and vcxitems files
- Simplify test CMakeLists.txt files
- Fix CMake conventions in packaging scripts

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep the before.targets toolset selection deterministic on newer
Visual Studio hosts, but only set the MIP props fallback when a
consumer has not already chosen a toolset. While addressing the
MSBuild review comments, also point the optional module test
conditions and source paths at the real lib/modules locations and
use CPACK_PACKAGE_FILE_NAME for the RPM status message.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Quote the optional ECS test.json configure_file input and output paths so CMake handles source or build directories that contain spaces.

Files changed:

- tests/functests/CMakeLists.txt

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PR microsoft#1416 inadvertently regressed PR microsoft#1415 by removing
-fno-finite-math-only from the three Unix REL_FLAGS branches and adding
-Wno-nan-infinity-disabled to silence the Clang diagnostic. That left
release builds using -ffast-math without preserving the NaN/Inf
semantics needed by nlohmann::json and SQLite paths.

Remove -ffast-math entirely from the GCC, AppleClang, and generic Clang
release flags rather than relying on -fno-finite-math-only to partially
undo it. This SDK is not floating-point compute-bound; its hot paths are
string, Bond/JSON serialization, HTTP I/O, and SQLite reads/writes.

Avoiding -ffast-math:
- preserves std::isnan/std::isinf behavior for JSON and storage code,
- avoids compiler/runtime fast-math side effects such as x86 GCC's
  crtfastmath.o changing MXCSR FTZ/DAZ behavior process-wide, and
- aligns with SQLite's guidance to avoid fast-math.

This subsumes microsoft#1415's partial mitigation and aligns with microsoft#1392's intent,
extended to GCC because GCC fast-math has the same broad assumptions and
runtime side effects. Also remove -Wno-nan-infinity-disabled because the
warning should not be suppressed once the cause is gone.

Validation:
- CMake Release build of UnitTests on macOS arm64 (AppleClang 21).
- UnitTests passed on macOS arm64.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Avoid carrying a confusing note about warning flags that are not used, and keep CMakeLists.txt focused on active compiler settings.

Files changed:

- CMakeLists.txt

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001
bmehta001 force-pushed the bhamehta/code-cleanup branch from 1d09c10 to 1cfe326 Compare May 22, 2026 20:09
bmehta001 and others added 16 commits May 28, 2026 03:26
Copilot review on PR microsoft#1416 flagged 7 unquoted ${CMAKE_SOURCE_DIR}/...
paths inside list(APPEND SRCS ...) calls. CMake splits unquoted
arguments on whitespace, so a checkout path containing spaces (very
common on Windows: C:\Users\First Last\source\...) would fragment a
single source path into multiple list elements and the test wouldn't
compile.

Quote every ${CMAKE_SOURCE_DIR}/.../*.cpp path inside
list(APPEND SRCS ...) in both tests/unittests/CMakeLists.txt and
tests/functests/CMakeLists.txt (23 paths total, including the
privacyguard + sanitizer blocks Copilot didn't cite but that share the
same hazard).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot review on PR microsoft#1416 flagged that mapping VisualStudioVersion >= 18.0
to PlatformToolset v145 is a guess: VS 2025/2026/etc. has not shipped, the
toolset moniker for that release may differ, and pinning to a non-existent
toolset name causes MSBuild to fail early with "unknown PlatformToolset"
on any machine that does pick up that VS version.

Drop the v145 mapping from both before.targets and build.MIP.props. Future
VS versions fall through to the v141 fallback (same behavior as before
PR microsoft#1416, just one less wrong-toolset-name failure mode). Add explicit
mappings for new toolsets as they actually ship.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The top-level CMakeLists.txt asks for C++14 (CMAKE_CXX_STANDARD 14, set in
2018) but then re-injects -std=c++11 into CMAKE_CXX_FLAGS for both Release
and Debug Unix builds (set in 2019 when gcc-4.x / gcc-5.x C++14 support was
still spotty). That explicit flag wins, so the Unix builds have actually
been C++11 ever since, contradicting the stated standard.

This is a vestige: gcc-5+, clang-3.4+, and MSVC 2015+ all support C++14
fully, and Android (lib/android_build/) already sets CMAKE_CXX_STANDARD 14
explicitly. Removing the override aligns the Unix CMake builds with the
intended standard.

- Drop the stale gcc-4.x/5.x caveat comment and add CMAKE_CXX_STANDARD_REQUIRED
  ON so the build fails loudly on toolchains that cannot do C++14, rather
  than silently falling back.
- Drop the -std=c++11 fragment from both REL and DBG CMAKE_CXX_FLAGS lines.
  C and C11 are left alone (C11 remains the C floor).

Not touched in this commit:
- examples/**/CMakeLists.txt and wrappers/obj-c/CMakeLists.txt also pin
  -std=c++11. Those are standalone sample/wrapper build scripts not wired
  via add_subdirectory and not built by any CI workflow; their pins are
  intentional statements of each sample's minimum and can stay.
- This commit does not bump the project to C++17. Cleanup ideas like
  cpp_client_telemetry_modules#302 (std::string::data() -> char*) require
  C++17 and are still out of scope.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Derive package version components from MATSDK_BUILD_VERSION so CPack packages stay aligned with the SDK version.

Use PROJECT_SOURCE_DIR and PROJECT_BINARY_DIR for test CMake paths so the SDK remains subproject-friendly.

Files changed:

- CMakeLists.txt

- tools/MakeDeb.cmake

- tools/MakeRpm.cmake

- tools/MakeTgz.cmake

- tests/functests/CMakeLists.txt

- tests/unittests/CMakeLists.txt

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Validate MATSDK_BUILD_VERSION before deriving CPack version components and remove now-dead DAYNUMBER calculations from package scripts.

Files changed:

- CMakeLists.txt

- tools/MakeDeb.cmake

- tools/MakeRpm.cmake

- tools/MakeTgz.cmake

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The previous merge from main left a conflict marker block in lib/CMakeLists.txt's trailing commented-link section. Remove the marker block and keep the cleaned-up file content.

Files changed:

- lib/CMakeLists.txt

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep package script version cleanup out of the CMake cleanup PR; package version alignment can be handled separately before the next release.

Files changed:

- CMakeLists.txt

- tools/MakeDeb.cmake

- tools/MakeRpm.cmake

- tools/MakeTgz.cmake

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add an explicit VS2026/v145 command-line build entry alongside the existing VS2022/v143 script so Windows consumers can choose the right supported toolset without guessing from MSBuild internals.

Files changed:
- docs/cpp-start-windows.md
- build-all-v145.bat
- tools/vcvars.cmd
- Solutions/before.targets

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
zlib/CMakeLists.txt still targets old CMake compatibility, but add_compile_definitions requires newer CMake. Restore add_definitions for the Windows preprocessor definitions so the zlib subproject remains compatible with the branch's stated CMake floor.

Validation:
- cmake configure with CMAKE_POLICY_VERSION_MINIMUM=3.5

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants