Query cache always missed in session having altered the entities #1731

igitur · 2018-06-04T16:59:40Z

This is my first fix to NHibernate. Please let me know what else is required on this PR. Ideally, I'd like this patch to be included on the 4.1 branch. Anything I can do to make that easier?

hazzik · 2018-06-04T21:52:26Z

@fredericDelaporte 5.1.3 or 5.2?

hazzik · 2018-06-04T21:53:42Z

Also, I think that async tests are missing.

igitur · 2018-06-04T22:03:29Z

Ah, yes, I'll add async tests.

Comments on the possibility of backporting to 4.1?

hazzik · 2018-06-04T22:06:43Z

4.1 unlikely. What’s holding you from updating to 5.x?

igitur · 2018-06-04T22:12:51Z

Stuck on net452 for a while.

hazzik · 2018-06-04T23:43:40Z

@fredericDelaporte odd semi-unrelated question (but I was thinking about it for a while): .net 4.5 support in 5.2, yeah/nah?

I did initial analysis: the only thing is missing is AsyncLocal<>. It has 2 usages:

AsyncLocalSessionContext - this has to go into the void;
AdoNetWithSystemTransactionFactory._bypassLock - this can be replaced with CallContext.LogicalGetData/CallContext.LogicalSetData surrounded to compilation directive.

A rationale for that: to get a better user acquisition on a newer version.

igitur · 2018-06-05T08:50:35Z

Async test added.

fredericDelaporte · 2018-06-05T11:21:17Z

Async test added.

Hum, have you missed a git add?

hazzik · 2018-06-05T11:22:00Z

It seems it's manual addition

fredericDelaporte · 2018-06-05T11:24:43Z

@fredericDelaporte odd semi-unrelated question (but I was thinking about it for a while): .net 4.5 support in 5.2, yeah/nah?

I am in favor of adding it. The main drawback will be the increase in the NuGet size. And should we start providing multiple targets in the SourceForge package? (Or on the contrary, drop SourceForge publishing?)

hazzik · 2018-06-05T11:27:52Z

https://twitter.com/AlfredoCambera/status/1002966376468770816

igitur · 2018-06-05T11:38:15Z

Hum, have you missed a git add?

No, I added it as a new test method in Fixture.cs.

Ah, I just re-read the contributing guidelines and I see the part about using the async code generator. I'll redo the async part using the generator.

igitur · 2018-06-05T11:50:01Z

In other words @igitur, have you used the async generator like explained here?

Now I have.

hazzik · 2018-06-05T11:51:49Z

src/NHibernate.Test/NHSpecificTest/GH1730/Fixture.cs

+	{
+		protected override void Configure(Configuration configuration)
+		{
+			configuration.SetProperty("show_sql", "true");


is this required?

hazzik · 2018-06-05T11:52:12Z

src/NHibernate.Test/NHSpecificTest/GH1730/Fixture.cs

+		protected override void Configure(Configuration configuration)
+		{
+			configuration.SetProperty("show_sql", "true");
+			configuration.SetProperty("generate_statistics", "true");


Please use a property from Cfg.Environment

fredericDelaporte · 2018-06-05T13:18:16Z

5.1.3 or 5.2?

I am more for 5.2. Especially since the opener would need this down to 4.1, which is quite unlikely we will do for a trouble which is not a serious regression. Moreover if we do support Fx4.5 in 5.2, that may suit him as I understand his need for having it in 4.1.

igitur · 2018-06-05T13:19:33Z

If you do plan to support net45 in 5.2, I can definitely wait until then.

fredericDelaporte · 2018-06-05T14:06:27Z

Still on the unrelated subject, TransactionScopeAsyncFlowOption seems to be there only from 4.5.1, so that will be net451 rather than net45. #ifdef-ing that would create bugs with async-await.

hazzik · 2018-06-05T14:27:40Z

Tried to do .NET 4.5 and found more issues: Task.FromCancelled(...), Task.FromException(...), Task.CompletedTask, and Array.Empty<T>() are missing. The array is easy to fix. Task is not so - it requires changes to AsyncGenerator. So, let's close the off-topic for now.

fredericDelaporte · 2018-06-05T14:52:34Z

So well, we are back to "where should we put that fix"... I still do not think we should backport it to 4.1. Of course @igitur could rebase this PR on 5.1.x for inclusion in 5.1.3 then he could create backport PRs cherry-picking the change in 5.0.x and 4.1.x, reducing the work on the team side.
But that would be still publishing two additional releases for a non-regression bug, which is time consuming. And this could incite more people to ask for more backports, which I would rather avoid.

igitur · 2018-06-05T14:55:19Z

Don't worry about the 4.1 request. I can use this as motivation to upgrade our apps to net461+.

Yes, I can rebase on 5.1.x if @hazzik is OK with that.

hazzik · 2018-06-05T22:26:06Z

Yes, 5.1.3 would be good. So, please rebase.

…ted the cache

igitur · 2018-06-06T08:30:39Z

Rebased on 5.1.x

hazzik · 2018-06-06T21:38:42Z

Thanks @igitur

igitur force-pushed the GH1730-query-cache-timestamp-fix branch from 72b71be to 857c1db Compare June 4, 2018 17:00

hazzik previously approved these changes Jun 4, 2018

View reviewed changes

hazzik added the t: Fix label Jun 4, 2018

igitur dismissed hazzik’s stale review via fd0220a June 5, 2018 07:00

igitur force-pushed the GH1730-query-cache-timestamp-fix branch from 857c1db to fd0220a Compare June 5, 2018 07:00

igitur force-pushed the GH1730-query-cache-timestamp-fix branch from fd0220a to a914425 Compare June 5, 2018 11:46

hazzik reviewed Jun 5, 2018

View reviewed changes

igitur force-pushed the GH1730-query-cache-timestamp-fix branch from a914425 to d044a5c Compare June 5, 2018 12:52

fredericDelaporte changed the title ~~Query Cache effective only after closing the session that created the cache~~ Query cache always missed in session having altered the entities Jun 5, 2018

hazzik previously approved these changes Jun 5, 2018

View reviewed changes

igitur dismissed hazzik’s stale review via 17a9961 June 6, 2018 08:28

igitur force-pushed the GH1730-query-cache-timestamp-fix branch from d044a5c to 17a9961 Compare June 6, 2018 08:28

igitur changed the base branch from master to 5.1.x June 6, 2018 08:29

GH1730 Query Cache effective only after closing the session that crea…

683df95

…ted the cache

igitur force-pushed the GH1730-query-cache-timestamp-fix branch from 17a9961 to 683df95 Compare June 6, 2018 08:30

hazzik approved these changes Jun 6, 2018

View reviewed changes

hazzik merged commit 683df95 into nhibernate:5.1.x Jun 6, 2018

hazzik added the r: Fixed label Jun 6, 2018

fredericDelaporte added this to the 5.1.3 milestone Jun 6, 2018

igitur deleted the GH1730-query-cache-timestamp-fix branch June 25, 2018 14:42

Query cache always missed in session having altered the entities #1731

Query cache always missed in session having altered the entities #1731

Uh oh!

Conversation

igitur commented Jun 4, 2018

Uh oh!

hazzik commented Jun 4, 2018

Uh oh!

hazzik commented Jun 4, 2018

Uh oh!

igitur commented Jun 4, 2018

Uh oh!

hazzik commented Jun 4, 2018

Uh oh!

igitur commented Jun 4, 2018

Uh oh!

hazzik commented Jun 4, 2018

Uh oh!

igitur commented Jun 5, 2018

Uh oh!

fredericDelaporte commented Jun 5, 2018

Uh oh!

hazzik commented Jun 5, 2018

Uh oh!

fredericDelaporte commented Jun 5, 2018

Uh oh!

hazzik commented Jun 5, 2018

Uh oh!

igitur commented Jun 5, 2018

Uh oh!

igitur commented Jun 5, 2018

Uh oh!

hazzik Jun 5, 2018

Choose a reason for hiding this comment

Uh oh!

igitur Jun 5, 2018

Choose a reason for hiding this comment

Uh oh!

hazzik Jun 5, 2018

Choose a reason for hiding this comment

Uh oh!

igitur Jun 5, 2018

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte commented Jun 5, 2018

Uh oh!

igitur commented Jun 5, 2018

Uh oh!

fredericDelaporte commented Jun 5, 2018

Uh oh!

hazzik commented Jun 5, 2018

Uh oh!

fredericDelaporte commented Jun 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

igitur commented Jun 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hazzik commented Jun 5, 2018

Uh oh!

igitur commented Jun 6, 2018

Uh oh!

hazzik commented Jun 6, 2018

Uh oh!

Uh oh!

fredericDelaporte commented Jun 5, 2018 •

edited

Loading

igitur commented Jun 5, 2018 •

edited

Loading