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.

Sign up for GitHub

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

Jump to bottom

Revisit concurrency detector #8305

Closed
ajcvickers opened this issue Apr 27, 2017 · 5 comments
Closed

Revisit concurrency detector #8305

ajcvickers opened this issue Apr 27, 2017 · 5 comments
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Milestone
2.0.0

Comments

@ajcvickers
Copy link
Contributor

Need to make sure:

  • If async paths are intended to be serialized, then make sure this works for all async paths--i.e. done at appropriate level
    • Consider overhead for doing this when there really isn't any async code in the async path--i.e. when Task.FromResult is used.
  • New code paths that have been added after the concurrency detector was introduced are making the right calls into the detector.
    • How do we make sure this stays up-to-date going forward?

See also #8026
@ajcvickers ajcvickers added this to the 2.0.0 milestone Apr 28, 2017
@anpete
Copy link
Contributor

anpete commented May 16, 2017

@ajcvickers We currently do the throwing check in the following:

  • ExecuteSqlCommand
  • ExecuteSqlCommandAsync
  • SaveChanges
  • SaveChangesAsync
  • MoveNext (sync query)

And we do the async waiting check (interleaving) only in:

  • MoveNextAsync (async query)

Obviously there are many more top-level APIs that we could add the check too. Do we want to do that?

anpete added a commit that referenced this issue May 16, 2017
@anpete
Fixes for #8305 and possibly flaky Test: AsyncQueryTestBase.Mixed_syn…
8d96835 
…c_async_query (part of #7160)

- Moved query async Semaphore to RelationalConnection so multiple top-level queries can be serialized.
- Moved active query buffer management to RelationalConnection so buffering works across multiple top-level queries.
- Renamed IValueBufferCursor to IBufferable
@anpete anpete mentioned this issue May 16, 2017
Merged
anpete added a commit that referenced this issue May 16, 2017
@anpete
Fixes for #8305 and possibly flaky Test: AsyncQueryTestBase.Mixed_syn…
724a367 
…c_async_query (part of #7160)

- Moved query async Semaphore to RelationalConnection so multiple top-level queries can be serialized.
- Moved active query buffer management to RelationalConnection so buffering works across multiple top-level queries.
- Renamed IValueBufferCursor to IBufferable
anpete added a commit that referenced this issue May 16, 2017
@anpete
Fixes for #8305, #8026 and possibly flaky Test: AsyncQueryTestBase.Mi…
07736bd 
…xed_sync_async_query (part of #7160)

- Moved query async Semaphore to RelationalConnection so multiple top-level queries can be serialized.
- Moved active query buffer management to RelationalConnection so buffering works across multiple top-level queries.
- Renamed IValueBufferCursor to IBufferable
@anpete
Copy link
Contributor

anpete commented Jun 8, 2017

ping @ajcvickers

@ajcvickers ajcvickers self-assigned this Jun 9, 2017
@ajcvickers
Copy link
Contributor Author

Design meeting notes:

  • We will not attempt to make the context completely thread-safe since there is too much complexity in the implementation and the extra overhead for the normal single-threaded use may be too much.
  • @ajcvickers to propose other places to add the check. It doesn't need to be added everywhere, but we want to add in places where it is likely to get hit so that apps will more likely throw a good exception rather than just some random exception from multi-threaded access. (Note to self--context initialization?)
  • @ajcvickers to do some tests to make sure common patterns will hit the check

@ajcvickers ajcvickers reopened this Jun 15, 2017
@AndriySvyryd
Copy link
Member

Also the concurrency detector doesn't fire reliably on .First(), see the Throws_on_concurrent_query_first() test.

@AndriySvyryd
Copy link
Member

See also #8865

ajcvickers added a commit that referenced this issue Jul 5, 2017
@ajcvickers
More tests for ConcurrencyDetector
430d80a 
Issue #8305

Also, updated text for recursive call to OnConfiguring since in my investigation this was often the first error seen in invalid multi-threading scenarios.
@ajcvickers ajcvickers mentioned this issue Jul 5, 2017
Merged
ajcvickers added a commit that referenced this issue Jul 5, 2017
@ajcvickers
More tests for ConcurrencyDetector
255ba82 
Issue #8305

Also, updated text for recursive call to OnConfiguring since in my investigation this was often the first error seen in invalid multi-threading scenarios.
ajcvickers added a commit that referenced this issue Jul 5, 2017
@ajcvickers
More tests for ConcurrencyDetector
49875d6 
Issue #8305

Also, updated text for recursive call to OnConfiguring since in my investigation this was often the first error seen in invalid multi-threading scenarios.
@ajcvickers ajcvickers added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Jul 5, 2017
@ajcvickers ajcvickers removed their assignment Sep 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Projects
None yet
Milestone
2.0.0
Development

No branches or pull requests
3 participants
@anpete @ajcvickers @AndriySvyryd