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

S2222: Branch on createdNew parameter on Mutex constructor #6994

Merged
merged 16 commits into from
Apr 3, 2023

Conversation

martin-strecker-sonarsource
Copy link
Contributor

@martin-strecker-sonarsource martin-strecker-sonarsource commented Mar 27, 2023

Mutex has a ctor out parameter bool createdNew which indicates if the mutex was created and a lock is held. This parameter was ignored but we need to branch on it.

Replaces #6958

@martin-strecker-sonarsource martin-strecker-sonarsource changed the title S2222: S2222: Branch on createdNew parameter on Mutex constructor Mar 27, 2023
@martin-strecker-sonarsource
Copy link
Contributor Author

I looked into the ITs and there are no S2222 related changes.

@@ -23,8 +23,6 @@ public void Noncompliant(Foo foo)
var m3 = Mutex.OpenExisting("x");
m3.WaitOne(); // Noncompliant

var m4 = new Mutex();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this removal, the test fails. This is likely caused by the new branching and the explosion of states.

Copy link
Contributor

@pavel-mikula-sonarsource pavel-mikula-sonarsource left a comment

Choose a reason for hiding this comment

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

The TrackedSymbol change doesn't look intentional to me

Copy link
Contributor

@pavel-mikula-sonarsource pavel-mikula-sonarsource left a comment

Choose a reason for hiding this comment

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

Minor polishing

Copy link
Contributor

@pavel-mikula-sonarsource pavel-mikula-sonarsource left a comment

Choose a reason for hiding this comment

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

Final polish of the extension base

Copy link
Contributor

@pavel-mikula-sonarsource pavel-mikula-sonarsource left a comment

Choose a reason for hiding this comment

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

LGTM

/// <summary>
/// Returns the argument value corresponding to <paramref name="parameterName"/>. For <see langword="params"/> parameter an IArrayCreationOperation is returned.
/// </summary>
private static IOperation ArgumentValue(this ImmutableArray<IOperation> arguments, string parameterName)
Copy link
Contributor

Choose a reason for hiding this comment

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

Even when it's private, I wouldn't make it an extension.

Suggested change
private static IOperation ArgumentValue(this ImmutableArray<IOperation> arguments, string parameterName)
private static IOperation ArgumentValue(ImmutableArray<IOperation> arguments, string parameterName)

@sonarcloud
Copy link

sonarcloud bot commented Apr 3, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sonarcloud
Copy link

sonarcloud bot commented Apr 3, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@martin-strecker-sonarsource
Copy link
Contributor Author

Peach validation: One issue disappeared. It was related to Monitor and not to Mutex. It was flaky before so it is probably related to something else:

image

It looks like a true negative to me.

		public void AddEntries(IList<IndexEntry> entries) {
			Ensure.NotNull(entries, "entries");
			Ensure.Positive(entries.Count, "entries.Count");

			var collection = entries.Select(x => new IndexEntry(GetHash(x.Stream), x.Version, x.Position)).ToList();

			// only one thread at a time can write
			Interlocked.Add(ref _count, collection.Count);

			var stream = collection[0].Stream; // NOTE: all entries should have the same stream
			SortedList<Entry, byte> list = null;
			try {

			if (!_hash.TryGetValue(stream, out list)) {
				list = new SortedList<Entry, byte>(MemTableComparer);
				if (!Monitor.TryEnter(list, 10000)) // <-- This issue disappeared
					throw new UnableToAcquireLockInReasonableTimeException();
				_hash.AddOrUpdate(stream, list,
					(x, y) => {
						throw new Exception("This should never happen as MemTable updates are single-threaded.");
					});
			} else{
				if (!Monitor.TryEnter(list, 10000))
					throw new UnableToAcquireLockInReasonableTimeException();
			}

				for (int i = 0, n = collection.Count; i < n; ++i) {
					var entry = collection[i];
					if (entry.Stream != stream)
						throw new Exception("Not all index entries in a bulk have the same stream hash.");
					Ensure.Nonnegative(entry.Version, "entry.Version");
					Ensure.Nonnegative(entry.Position, "entry.Position");
					list.Add(new Entry(entry.Version, entry.Position), 0);
				}
			} finally {
				if(list != null)
					Monitor.Exit(list);
			}
		}

@martin-strecker-sonarsource martin-strecker-sonarsource added this to the 8.56 milestone Apr 18, 2023
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