Skip to content

Concurrency token checks in Update and Delete methods of InMemoryTable #10158

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
Merged
merged 1 commit into from
Nov 1, 2017
Merged

Concurrency token checks in Update and Delete methods of InMemoryTable #10158

merged 1 commit into from
Nov 1, 2017

Conversation

shaynevanasperen
Copy link
Contributor

@shaynevanasperen shaynevanasperen commented Oct 25, 2017 •
edited
Loading

  • The code builds and tests pass (verified by our automated build checks)
  • Commit messages follow this format
    Summary of the changes
    - Detail 1
    - Detail 2

    Fixes #10157

@dnfclas
Copy link

dnfclas commented Oct 25, 2017

@shaynevanasperen,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla2.dotnetfoundation.org.

It will cover your contributions to all .NET Foundation-managed open source projects.
Thanks,
.NET Foundation Pull Request Bot

@dnfclas
Copy link

dnfclas commented Oct 25, 2017

@shaynevanasperen, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, .NET Foundation Pull Request Bot

ajcvickers
ajcvickers reviewed Oct 25, 2017
View reviewed changes
src/EFCore.InMemory/Properties/InMemoryStrings.resx Outdated
@@ -128,4 +128,7 @@
<data name="UpdateConcurrencyException" xml:space="preserve">
<value>Attempted to update or delete an entity that does not exist in the store.</value>
</data>
<data name="UpdateConcurrencyTokenException" xml:space="preserve">
<value>A conflict was detected on one or more properties configured as a concurrency token.</value>
Copy link
Contributor

Choose a reason for hiding this comment

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

We always try to include information about which properties caused the conflict.

It would also be useful to tell people what the conflicting values are, but only if sensitive data logging is enabled. However, I think we would still take the PR without this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ajcvickers Thanks for your feedback. I've now updated it to include this information.

ajcvickers
ajcvickers reviewed Oct 25, 2017
View reviewed changes
src/EFCore.InMemory/Storage/Internal/InMemoryTable.cs Outdated
@@ -89,6 +105,18 @@ public virtual void Update(IUpdateEntry entry)
}
}

private static bool ValueEquality(object val1, object val2)
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the scenario that requires this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ajcvickers I'd imagine it would be every scenario? The reason is that when comparing boxed value types, they are not automatically considered equal, even if the values "inside the boxes" are equal. I got the code snippet from this Stack Overflow post: https://stackoverflow.com/questions/6668697/boxed-value-type-comparisons

Copy link
Contributor

Choose a reason for hiding this comment

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

@shaynevanasperen Looks like this would only be an issue where comparing different types that have been boxed. Does this happen in the in-memory provider? If so, is there a test that fails or can one be written? (It's not that this is necessarily bad, but we have lots of code that rely on exact type comparisons, often in dynamically compiled code, so I would like to understand more clearly why this is needed in case is is masking a more fundamental bug.)

Copy link
Contributor Author

@shaynevanasperen shaynevanasperen Oct 31, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@ajcvickers Without this, my code doesn't work and always throws the DbUpdateConcurrencyException. Most of the existing tests in EFCore.Specification.Tests/UpdatesTestBase.cs fail without this code (when running the EFCore.InMemory.FunctionalTests/UpdatesInMemoryTest.)

Like this one for instance:
https://github.com/aspnet/EntityFrameworkCore/pull/10158/files#diff-125fbbaa7088e78da7b5c772bb2ddb65R35

In that case it is comparing a boxed decimal to another boxed decimal and even when the values are the same the equality check fails, so I had to write this.

Copy link
Contributor

Choose a reason for hiding this comment

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

@shaynevanasperen I pulled your code and changed ...en && !ValueEquality(_ro... in both places to ...en && !Equals(_ro..., commented out the ValueEquality method, and all the tests pass for me. It would be great to figure out why we are seeing different behavior here, since the rest of the PR looks great.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ajcvickers That's bizarre! I originally wrote it as ...en && _rows[key][index] != entry.GetOriginalValue(properties[index]), but that didn't work.

Isn't x != y functionally equivalent to !Equals(x, y)?

I'll see if I can reproduce it now and then get back to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops! I feel so silly now. x != y is a reference equality check. For some reason, I wrongly thought that if falls back to doing a value equality check if the reference equality check returns false. In fact it is the other way around. A value equality check first does a reference equality comparison and then falls back to value comparison.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ajcvickers I've just updated the code to use ...en && !Equals(_ro... in both places, and I've removed the ValueEquality method.

ajcvickers
ajcvickers suggested changes Oct 25, 2017
View reviewed changes
Copy link
Contributor

@ajcvickers ajcvickers left a comment

Choose a reason for hiding this comment

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

Se inline comments:

  • Add at least property names to the exception message
  • Clarify the use of converting equality code

@ajcvickers ajcvickers self-assigned this Oct 25, 2017
@shaynevanasperen shaynevanasperen force-pushed the Issue10157 branch 2 times, most recently from 8aa1146 to 5bb241a Compare October 30, 2017 00:24
@ajcvickers ajcvickers merged commit e70d7da into dotnet:dev Nov 1, 2017
@ajcvickers
Copy link
Contributor

Thanks for the contribution!

@tsangste tsangste mentioned this pull request Jun 2, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajcvickers ajcvickers ajcvickers approved these changes

Assignees

@ajcvickers ajcvickers

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@shaynevanasperen @dnfclas @ajcvickers