Skip to content

Conversation

AR-May
Copy link
Member

@AR-May AR-May commented Jul 4, 2025

Related to #11828.

This spec defines the API contract for the new kind of Task that would be safe to use in multithreaded mode of execution.

@AR-May AR-May self-assigned this Jul 9, 2025
@AR-May AR-May force-pushed the dev/AR-May/thread-safe-tasks-spec branch from 21b22a3 to 0914b4e Compare July 9, 2025 18:09
@AR-May
Copy link
Member Author

AR-May commented Jul 15, 2025

We found probably some better way to deal with file system rather than re-implement it. Re-wrote our concept.

@AR-May AR-May force-pushed the dev/AR-May/thread-safe-tasks-spec branch from 61b490d to cab9556 Compare August 27, 2025 12:52
…ath without providing absolute path for task authors
@AR-May AR-May marked this pull request as ready for review September 25, 2025 13:53
@Copilot Copilot AI review requested due to automatic review settings September 25, 2025 13:53
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a comprehensive specification for thread-safe tasks in MSBuild's multithreading execution model. The purpose is to enable tasks to run concurrently within a single MSBuild process while maintaining safety and correctness.

  • Defines thread-safe task capabilities through interfaces and attributes
  • Introduces TaskEnvironment API for safe access to process-level state
  • Provides detailed API analysis for identifying threading issues in existing code

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
thread-safe-tasks.md Main specification document defining thread-safe task interfaces, TaskEnvironment API, and implementation approaches
thread-safe-tasks-api-analysis.md Comprehensive reference of .NET APIs that are problematic for thread-safe tasks with recommendations
multithreaded-msbuild.md Minor correction updating TaskExecutionContext reference to TaskEnvironment

@AR-May AR-May changed the title [WIP] Thread-Safe Tasks spec Thread-Safe Tasks spec Sep 25, 2025
internal AbsolutePath(string path, bool ignoreRootedCheck) { }
public AbsolutePath(string path); // Checks Path.IsPathRooted
public AbsolutePath(string path, AbsolutePath basePath) { }
public static implicit operator string(AbsolutePath path) { }
Copy link
Member

Choose a reason for hiding this comment

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

I feel like an explicit conversion operator from string -> AbsolutePath would be a good shorthand as well for large codebases.

Copy link
Member Author

@AR-May AR-May Oct 1, 2025

Choose a reason for hiding this comment

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

We might consider adding it later if the need for this shortcut would appear.This conversion would need to check that the string is indeed already an absolute path, otherwise we do not have enough data to make a conversion. I am afraid it might be still confusing that the conversion may fail if the path is not rooted.

Copy link
Member

Choose a reason for hiding this comment

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

I feel like it should also be able to resolve shorthanded paths like ./somefile.txt to it's absolute path as well (by replacing the . in the string with the current directory in the ctor.

Copy link
Member Author

Choose a reason for hiding this comment

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

The conversion operator would not have enough context to access correct current directory - so the only way to do that now would be using the TaskEnvironement object created specifically so it has this context and allows the absolute path resolution. Besides, parsing dots in paths feels kind of too hacky to me.

However, MSBuild tasks often work not with stings but with MSBuild items, which should have enough data in many cases, and we might consider having conversion operators for them in version 2.

Copy link
Member

@baronfel baronfel left a comment

Choose a reason for hiding this comment

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

Approving so we can get this merged - we can always revise it as we go through implementation.

@AR-May AR-May merged commit f87a18c into main Oct 1, 2025
9 checks passed
@AR-May AR-May deleted the dev/AR-May/thread-safe-tasks-spec branch October 1, 2025 15:30
@AR-May
Copy link
Member Author

AR-May commented Oct 1, 2025

Merged the PR as we are feeling pretty good about the decisions made for this spec.
Feel free to open issues / discussions or still ping me here if you need to discuss this spec.

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.

8 participants