-
-
Notifications
You must be signed in to change notification settings - Fork 200
Enable ThreadSafetyChecks by default on all tests #622
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
base: main
Are you sure you want to change the base?
Conversation
|
I tried to validate the thread-safety behavior introduced/affected by this PR by writing a dedicated test using Example: `[Fact]`
`public` `async` `Task` `InvalidOperation()`
`{`
`await` `using` `var` `session` `=` `_store.CreateSession();`
`var` `p1` `=` `new` `Person();`
`var` `p2` `=` `new` `Person();`
`var` `p3` = `new` `Person();`
`await` `session.SaveAsync(p1);`
`await` `session.SaveAsync(p2);`
`await` `session.SaveAsync(p3);`
`await` `session.SaveChangesAsync();`
`await` `Assert.ThrowsAsync<InvalidOperationException>(async` `()` `=>`
`{`
`await` `Task.WhenAll(`
`Task.Run(()` `=>` `session.SaveChangesAsync()),`
`Task.Run(()` `=>` `session.GetAsync<Person>(p1.Id))`
`);`
`});`
`}`
I ran this test with SQLite with EnableThreadSafetyChecks = true and false, and observed no difference: no exception
is thrown in either case.
Unless I missed something, I don’t see a behavior change here regarding concurrent access detection.
Additional note: running the same test against SQL Server (2017) does throw an InvalidOperationException, while SQLite
does not. This seems to be provider-specific rather than related to the changes in this PR.
After further testing, concurrent access on the same session does not consistently throw an exception.
The behavior is provider- and timing-dependent, and exceptions cannot be relied upon for validating thread safety..with EnableThreadSafetyChecks = true and false
I also wrote this Valid test and there of course no exceptions (with EnableThreadSafetyChecks = true and false) :
`[Fact]`
`public` `async` `Task` `ValidOperation()`
`{`
`await` `using` `var` `session` `=` `_store.CreateSession();`
`var` `p1` `=` `new` `Person();`
`var` `p2` `=` `new` `Person();`
`var` `p3` `=` new `Person();`
`await` `session.SaveAsync(p1);`
`await` `session.SaveAsync(p2);`
`await` `session.SaveAsync(p3);`
`await` `session.SaveChangesAsync();`
`var` `ex0` `=` `await` `Assert.ThrowsAsync<InvalidOperationException>(async` `()` `=>`
`{`
`await`
`Task.Run(()` `=>` `session.GetAsync<Person>(p1.Id));`
`});`
`_output.WriteLine($"Exception` `Valid:` `{ex0.GetType().FullName}");`
`_output.WriteLine($"Exception` `messageValid:` `{ex0.Message}");`
}
"Assert.Throws() Failure: No exception was thrown"
"Expected: typeof(System.InvalidOperationException)" |
|
To conclude on this PR, and after testing on both SQLite and SQL Server for public virtual async Task ShouldDetectThreadSafetyIssues(): I could not observe any functional difference compared to the original branch: UseThreadSafetyChecks(bool) is simply a wrapper around EnableThreadSafetyChecks. The exceptions, messages, and stack traces are identical. Test outcomes are the same before and after the PR. At this stage, the PR appears to mainly provide API refactoring / fluent configuration improvements, without introducing new behavior or additional test coverage. Please let me know if there is an intended functional change or scenario I may have missed. |
I think that this is expected. The goal of this PR was to have the feature enabled for all our tests so we can detect more issues with the current code or the tests themselves. The flag ensures a session is not used concurrently by two threads. So ideally just setting it up should not fail any existing test. If it does then either we need to check the test is right, or there is a bug in yessql that it exposes. |
|
I removed the EnableThreadSafetyChecks test:
So since all the tests are running with the feature, I assume this is already well tested. At least covered by tests (not really ensuring it actually works). |
|
I also renamed the extension methods as I felt that |
No description provided.