Fix bitwise functions thread safety #1670

fredericDelaporte · 2018-04-26T12:20:42Z

Functions instances are hold in the dialect, shared among sessions spawned from the same factory. They must be thread safe.

Implementation moved to its correct directory by the way.

Fixes #1355

hazzik · 2018-04-26T23:37:28Z

src/NHibernate/Dialect/Function/BitwiseFunctionOperation.cs

+
+		public SqlString Render(IList args, ISessionFactoryImplementor factory)
+		{
+			var filteredArgs = new Queue();


Why would you need a queue here?

I have done the fix in a "minimal implementation changes" way, mainly fixing the fact that putting some state without any locks in an instance shared by many threads cannot be thread safe. So I have kept the previous logic using a queue.

Could you please remove it? Adding something to the queue which later is read from it just in the next block of code is an useless operation.

Not using it forces to add one more comma to the buffer then remove it. We need a filtered list if you wish to reliably avoid adding a comma in excess.

hazzik · 2018-04-26T23:38:30Z

src/NHibernate/Dialect/Function/BitwiseNativeOperation.cs

+		public SqlString Render(IList args, ISessionFactoryImplementor factory)
+		{
+			var sqlBuffer = new SqlStringBuilder();
+			var arguments = new Queue(args);


Again, why?

hazzik · 2018-04-26T23:59:19Z

I understand that this is just a refactoring of a previous work. However, for me it looks incorrect. Could this be reworked? (Preferably with unit tests)

fredericDelaporte · 2018-04-27T10:17:51Z

I do not call a fix a refactoring. The fact that current implementation is not thread safe seems obvious to me, as seems removing all mutable state to be a proper fix for achieving thread safety of instances. Now am I wrong considering function instances must be thread safe? Looking at the code, I do not think so.

Conceiving a test reliably demonstrating the thread safety failure while using sessions (so proving that thread safety is an issue) may be no easy task. The call to function rendering is just a very small part of the query creation processing, so having them occurring concurrently may be hard. There is also the query plan caching which may mitigate the trouble.

These functions are otherwise covered by some tests (Linq.WhereTests.BitwiseQuery/2/3, NH1192, NH1775, NH1831.)

fredericDelaporte · 2018-04-27T13:21:47Z

Anyway, I have added more tests and made one which demonstrates the thread safety issue. (At least on my machine, it succeeds seldom without the fix.)
I have also split the file move in its own commit, and added a commit for fixing PostgreSQL failing for xor (#1673).

fredericDelaporte · 2018-04-27T13:23:04Z

src/NHibernate.Test/TestCase.cs

@@ -447,6 +447,7 @@ protected DateTime RoundForDialect(DateTime value)
 				{"locate", new HashSet<System.Type> {typeof (SQLiteDialect)}},
 				{"bit_length", new HashSet<System.Type> {typeof (SQLiteDialect)}},
 				{"extract", new HashSet<System.Type> {typeof (SQLiteDialect)}},
+				{"bxor", new HashSet<System.Type> {typeof (SQLiteDialect)}},


Not supported by SQLite. http://sqlite.1065341.n5.nabble.com/XOR-operator-td98004.html

hazzik · 2018-04-28T15:18:52Z

I did not mean that your fix of thread safety is incorrect or there is no such issue. I called a "refactoring" the "minimal implementation changes". I was trying to say that there are some cases were and remain broken in the functions.

hazzik · 2018-04-28T15:24:23Z

src/NHibernate/Dialect/Function/BitwiseFunctionOperation.cs

+			var filteredArgs = new Queue();
+			foreach (var arg in args)
+			{
+				if (!IsParens(arg.ToString()))


It is not clear for me why this class worries about parens, and BitwiseNativeOperation does not? Where these assumptions came from? What would happen if some parens would end up as an arguments to BitwiseNativeOperation?

Thinking about it, it might be just an optimization, for removing parens which would be redundant otherwise, because the function call forcibly adds its own parens.

It appears to be a hack for the BitwiseQuery3 test. The args contains four elements instead of two, the first being actually an element, the second an opening parenthesis and the fouth a closing parenthesis. Outputing them all comma-separated could only fail. For this case ignoring the parens yield a good result. Could more elaborated queries yield more complex cases requiring more subtle handling? I do not know for sure. But fiddling with the query, I cannot get such a case.

The native case does not bother with it, because parens seem to appears only as second and fourth argument, while the operator is put only after the first argument. So it get suitably placed, and parens do not creates an issue there.

That seems a bit brittle, but it is needed and it seems there are no known failures with that.

hazzik · 2018-04-28T15:26:09Z

src/NHibernate/Dialect/Function/BitwiseNativeOperation.cs

+			if (_isNot == false)
+				AddToBuffer(arguments.Dequeue(), sqlBuffer);
+
+			AddToBuffer(string.Format(" {0} ", _sqlOpToken), sqlBuffer);


Should be just sqlBuffer.Add(string.Format(" {0} ", _sqlOpToken)), or better

sqlBuffer.Add(" ").Add(_sqlOpToken).Add(" ");

fredericDelaporte · 2018-04-28T15:54:50Z

Well, I did want to fix the direst trouble, thread safety, which can result in hard to debug troubles for users (rare intermittent failures). For the other shortcomings of the implementation, I have not considered them. If they cause issues, it is likely they are far easier to diagnose than a thread safety one. Or are you saying my changes introduce new issues?

This code comes from #280. @amroel, any remembering on the subject? Like about:

It is not clear for me why this class worries about parens, and BitwiseNativeOperation does not? Where these assumptions came from? What would happen if some parens would end up as an arguments to BitwiseNativeOperation?

fredericDelaporte · 2018-04-30T22:44:07Z

Thanks for the removal of the cherry-pick in 5.1.x. Now, do we simply merge this in 5.2, or should we fix the thread safety issue in previous versions?
I am more for the later, at least 5.1.x. This issue could bite anyone using a binary operator in a query in a multi-threaded application (so potentially many web applications), and that would likely cause infrequent failures, causing the application to lack reliability.

hazzik · 2018-04-30T22:47:48Z

Let's do 1st and 4th commits (squashed) into 5.1.x (we would need to exclude PostgreSQL from some tests as I understand).

Functions instances are hold in the dialect, shared among sessions spawned from the same factory. They must be thread safe.

fredericDelaporte · 2018-05-01T12:01:21Z

Base branch of PR changed, commit 1 & 4 squashed (and adjusted for ignoring PostgreSQL in bitwise xor tests), other commits dropped.

fredericDelaporte · 2018-05-01T12:06:02Z

src/NHibernate/Dialect/BitwiseNativeOperation.cs

@@ -15,28 +15,26 @@ public class BitwiseNativeOperation : ISQLFunction
 	{
 		private readonly string _sqlOpToken;
 		private readonly bool _isNot;


I have not kept the renaming to _isUnary in order to avoid a grey area breaking change (serialization) in a release version (5.1.x). Same for constructor argument name, kept as isNot instead of isUnary.

fredericDelaporte added t: Bug c: DataProviders / Dialects p: Major labels Apr 26, 2018

fredericDelaporte added this to the 5.2 milestone Apr 26, 2018

hazzik reviewed Apr 26, 2018

View reviewed changes

fredericDelaporte mentioned this pull request Apr 27, 2018

Bitwise xor treated as pow with PostgreSQL #1673

Closed

fredericDelaporte force-pushed the NonTHreadSafeFunction branch from 7f27122 to a7a9fb1 Compare April 27, 2018 13:19

fredericDelaporte commented Apr 27, 2018

View reviewed changes

hazzik reviewed Apr 28, 2018

View reviewed changes

fredericDelaporte force-pushed the NonTHreadSafeFunction branch from d96a282 to 4716635 Compare April 28, 2018 19:11

fredericDelaporte force-pushed the NonTHreadSafeFunction branch from 4716635 to 5c5b87e Compare May 1, 2018 11:52

fredericDelaporte changed the base branch from master to 5.1.x May 1, 2018 11:53

Fix thread safety of bitwise functions

f6270ec

Functions instances are hold in the dialect, shared among sessions spawned from the same factory. They must be thread safe.

fredericDelaporte force-pushed the NonTHreadSafeFunction branch from 5c5b87e to f6270ec Compare May 1, 2018 11:59

fredericDelaporte commented May 1, 2018

View reviewed changes

hazzik approved these changes May 1, 2018

View reviewed changes

hazzik modified the milestones: 5.2, 5.1.2 May 1, 2018

fredericDelaporte merged commit 231b080 into nhibernate:5.1.x May 1, 2018

fredericDelaporte deleted the NonTHreadSafeFunction branch May 1, 2018 21:45

fredericDelaporte added the r: Fixed label May 1, 2018

fredericDelaporte mentioned this pull request May 1, 2018

Release 5.1.2 #1664

Merged

hazzik mentioned this pull request May 2, 2018

NH-3928 - Random invalid SQL generated when using bitwise operators #1355

Closed

hazzik added t: Fix and removed t: Bug c: DataProviders / Dialects p: Major labels May 2, 2018

Fix bitwise functions thread safety #1670

Fix bitwise functions thread safety #1670

Uh oh!

Conversation

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

Uh oh!

Uh oh!

hazzik Apr 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hazzik Apr 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hazzik commented Apr 26, 2018

Uh oh!

fredericDelaporte commented Apr 27, 2018

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hazzik commented Apr 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hazzik Apr 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

fredericDelaporte commented Apr 30, 2018

Uh oh!

hazzik commented Apr 30, 2018

Uh oh!

fredericDelaporte commented May 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fredericDelaporte commented Apr 26, 2018 •

edited

Loading

hazzik Apr 26, 2018 •

edited

Loading

hazzik Apr 28, 2018 •

edited

Loading

fredericDelaporte commented Apr 27, 2018 •

edited

Loading

fredericDelaporte Apr 28, 2018 •

edited

Loading

hazzik Apr 28, 2018 •

edited

Loading

fredericDelaporte commented Apr 28, 2018 •

edited

Loading