Add protection against a pending message being consumed by multiple threads

[kendallb]

Add protection against a pending message being consumed by multiple threads somehow.

I believe there is a situation where multiple threads end up getting the same message as we have seen that in our logs. This is NOT supposed to happen with FOR UPDATE, as it is supposed to get a row level lock on the message in question so it should end up throwing a MySqlErrorCode.LockDeadlock transaction, which we ignore and continue. But somehow it looks like multiple threads grab the same message. Not sure if this is a MySQL bug, or a Percona XtraDB clustering issue or what, but we do seem to be seeing this. So to be 100% sure we are not grabbing a message already being processed in another thread, let's make sure leased_by is always null and bail if it is not.

---

Rebus is MIT-licensed. The code submitted in this pull request needs to carry the MIT license too. By leaving this text in, I hereby acknowledge that the code submitted in the pull request has the MIT license and can be merged with the Rebus codebase.

[kendallb]

Made some changes as we still need to handle the case where a message actually had a lease timeout. This does appear to have fixed out issues in production, so FOR UPDATE is not working as expected in the case of our MySQL cluster when all the threads line up and hit the DB at exactly the same instant.

[kendallb]

Still had some issues so I fixed it up and force pushed a better fix using a separate lock table. This seems to be working so we will try this in production for the next few days and monitor it.

[mookid8000]

(...) Not sure if this is a MySQL bug, or a Percona XtraDB clustering issue or what, (...)

It does not seem to be how MySQL works out of the box. I added a test here in an attempt to reproduce this behavior, but everything seemed to work as expected.

I am sorry, but while the PR might solve your specific problem, I don't think it belongs in the transport implementation, because if the database works as it should, then this should not be a problem.

My guess is that if you are using any kind of replicated setup with your database, and you want to use the database as a queue, it is imperative that you configure your Rebus instances to always connect to the master. If you allow reads from replicas, then I bet you will be in trouble.

If it turns out that this is not feasible, then I would suggest you maybe start considering using a real queueing system for your messaging needs.

[mookid8000]

Also, if you need to stay on the MySQL transport as it is now, it should be possible to fix your problem outside of the transport, using the same mechanism as you did in the PR here, just in your own message handler (or in an incoming message pipeline step).

[kendallb]

You will never be able to reproduce the problem in a test environment. I tried. But there is absolutely no way to fix this in production without this fix so it needs to be included.

I believe it has something to do with how transactions work with MySQL and the specific query in question in that it will not trigger deadlocks when a query is using FOR UPDATE and LIMIT. What MySQL attempts to do is serialize the transactions so that the end result is that they all work as expected so in theory the scenario we see is plausible but the timing would have to be absolutely spot on to trigger it. Which is impossible to craft a test for.

So this does fix the problem and IMHO is a clean solution but like I said it cannot be done outside of the transport. My assumption is that anyone else using this transport could easily run into the same issue.

[kendallb]

Can we open this one back up? We need it for sure and I think it would be a mistake to assume that our scenario is special and it would not happen to anyone else. I think it's just the way MySQL is wired and it does not work like SQL Server where their have better row locking with select support.

[mookid8000]

Hi @kendallb , if MySQL did not have row-level locking as required for the SELECT (...) FOR UPDATE to work, wouldn't you say it should be possible to reproduce the issue locally?

[kendallb]

Sure, it has row locking for FOR UPDATE and you can reproduce the exact problem easily if you simply remove FOR UPDATE. The problem is that without the ability to skip LOCKED rows in the select (which SQL Server has and also so does MySQL 8 I believe), MySQL does not guarantee that you will deadlock if two transactions attempt to modify the same row. All it will do is simply serialize the resulting transactions so they run one after the other and you get repeatable read. So if you remove FOR UPDATE, the first one gets the record, and makes its mods. Then the second one gets the SAME record but with the updated values from the first one when it then does it's update. I originally expected that if I removed FOR UPDATE the second one would always throw a transaction exception deadlock when you attempted to write the same record, but that is not how it works. I suspect since it sees you are writing the SAME columns it assumes it's OK to simply serialize the values.

So in the specific instance of what we are trying to achieve we do NOT want two threads to get the same record. With row level locking done at the select level AND the ability to skip locked rows, this would never happen. But even with FOR UPDATE and row level locking at the select level, and the threads line up exactly, there appears to be a situation where MySQL will let things run the same as without FOR UPDATE, which if you think about what the final result is (two threads activities serialized) MySQL is fine with is because the end result is the record was updated consistently after the two threads got properly serlialized.

But of course for the way we are trying to use this, that is entirely wrong as we cannot have two threads read the same record because the end result for us is not what ends up in the database, but what record we originally read as that is what is going to be processed as our next message. We cannot have that get read twice.

So the solution for MySQL 5.7 and earlier is we have to use a lock table to prevent two threads from reading the same rows. Until we are moved to MySQL 8 and I can test the theory of using the ability to skip locked rows, this is the only solution.

And while you could argue that this could be done externally with a distributed lock (which I already wrote support for in the MySQL transport) and a step handler, that is not a good solution as that assumes this problem is specific to me. It will not be, am 100% sure of it. Anyone else using MySQL 5.7 or earlier will have this problem.

Plus the lock table is locked at the message table ID level so it only locks out two threads if they happen to line up EXACTLY at the same time and were contending for the same record. So the performance would be a lot higher than if we always had to take an external lock and prevent multiple threads from calling into the transport at all times, because if the threads are spaced out even slightly, they will not get the same record.

[mookid8000]

Sure, it has row locking for FOR UPDATE and you can reproduce the exact problem easily if you simply remove FOR UPDATE. (...)

There's something I don't understand here.

If I remove FOR UPDATE then I wouldn't expect it to work, as I would clearly have left an opportunity for a race condition.

But if I understand things correctly, this is not what we're talking about here.

Aren't we talking about the transport seemingly being able to receive the same message twice on two nodes when running the transport as it is now on your Percona XtraDB cluster setup?

And again: I really don't know anything about Percona XtraDB, but a quick bingle for "percona xtradb select for update" brought me to an article about some intricacies around MVCC and SELECT ... FOR UPDATE, which I didn't fully understand, but I understood enough of it to think "yeah ok that's something to look out for".

[kendallb]

Right what I meant is if you remove FOR UPDATE you can reproduce the issue for every message. I originally thought this was a bug in MySQL but really it is not. As far as MySQL is concerned as long as the resulting values in the database are consistent after the transactions are all completed, it's happy. And when we get this condition the database values are correct because it has serialized the transaction order.

The problem is the way WE need it to work is not compatible with how MySQL transactions work because we only want one thread at a time reading the next record. I think the only way to guarantee it to work is to have the option to tell the select to ignore locked rows which is how it works in SQL server but that is not supported in MySQL 5.7 and earlier.

Essentially my solution is a way to simulate ignoring locked rows by using a lock table which we then join with against the main query. Although in reality it does not actually cause the threads to ignore the locked rows but rather all the other threads contending for the same row throw an exception and try again as soon as the first one in inserts the new lock entry.

Also bear in mind I only mentioned percona originally as that is the clustering solution we use. It likely would happen with regular MySQL also under the right conditions. And anyone using MySQL in a high availability environment is going to be using some kind of clustering solution. But I honestly don't think it's related to the cluster as all our write connections all hit the same node anyway. We only spread out the read traffic to other nodes and have HA set up so the write node can instantly fail over to another node if it goes down.

[mookid8000]

Right what I meant is if you remove FOR UPDATE you can reproduce the issue for every message.

But that would not produce the issue, that would simply produce (as in INTRODUCE) an issue - which would be quite expected! - which is why "FOR UPDATE" is there in the first place.

The problem is the way WE need it to work is not compatible with how MySQL transactions work because we only want one thread at a time reading the next record.

But relational databases have update locks to help with synchronization across nodes, so you shouldn't have to avoid concurrency like this, ever.

[kendallb]

Yes, but you are missing the critical part of what causes the problem for the Rebus transport. MySQL is not actually doing anything wrong, as it will try to seralize the transactions in different threads to avoid deadlocks where possible. It only throws a deadlock if the order if events within the transaction prevents it from doing that (and the extra lock table does just that). So from the MySQL perspective, it is perfectly fine if the two threads attempting to read the same record if one of them gets serialized. So the second one sees the updates from the first one and then continues on. Nice and serailized and if we only care about what ends up IN THE DATABASE, all is good. And that is all MySQL cares about.

However in our case we care not just about what ends up in the database, but also the temporal interleaving of those threads. It's not acceptable to have two threads read the same row as then we end up with multiple threads processing the same message. So this is not a bug in MySQL per se, but simply a problem with the way we are trying to use it. To fix it 100% we need a way to avoid locked rows in selects, which MySQL 5.7 and earlier do not have.

The alternative that would have worked for normal MySQL is to use a full table lock, but alas full table locks are not compatible with any clustering solutions like Percona XtraDB or MySQL Cluster.

[kendallb]

It has been a while and I need to follow up on this as I need to update our Rebus code. We cannot use Rebus.MySQL without these changes or it WILL end up processing multiple events at a time. The solution we implemented is what fixes it for us and using a lock table is really the only way to do this with MySQL. An update lock does not work for MySQL as mentioned above, because its is perfectly happen to let multiple threads run with a last in wins scenario if they are both updating the same data.

So can we revisit this please as I would very much like to migrate to official Rebus again, but without this we get multiple messages processed at once. And I am positive anyone else using this with MySQL and multiple threads running would be potentially having the same issues (same message being processed in two threads simulataneously which is not correct).

For now I will update all my code to the latest and re-base my changes but we must find a solution to this problem.

[mookid8000]

Hi @kendallb , if I remember this issue correctly, it's an issue that comes from using MySQL with an alternative storage engine that doesn't support "select ... for update"-style row locking and seems to just ignore the locking hint.

Since introducing a global locking mechanism in the transport would impact all other uses of the MySQL transport, I am reluctant to make the change in Rebus.MySql.

What I'll suggest is that you maintain a fork (e.g. like Rebus.MySql.PerconaXtraDb or something like that) that has Rebus.MySql as the upstream repo and adds the locking as a layer on top of that.

Do you think that would work for you?

[kendallb]

No, that is NOT going to work as that is the wrong analysis. All MySQL installations need this due to the way that it serializes transactions. It only cares about the final result being consistent, not that they get replayed IN ORDER. That is the crux of the problem and has nothing to with that fact we are using a Percona cluster. It's just how MySQL works, and internally Percona Cluster is just MySQL. The same issue would happen with any MySQL install or using Oracles MySQL cluster solution. All of them do fully support FOR UPDATE.

As mentioned previously, the way MySQL serialized the transactions is that it will NOT block a thread that is touching the same tables until it gets to the point where the results would not be consistent. So if two threads are writing to the exact same column in a database table row, the ordering of those two transactions completing is not important because one of them will have it's value overwritten. If you want to make sure it's consistent with respect to timing, you need to use a table lock. But table locks are not supported in cluster environments (does not matter if it's Percona or Oracle clustering table locks are not supported).

So the solution we are using is the same solution we use in other places in our code to implement distributed thread locks and that is to have a separate lock table that does nothing but make sure two threads trying to read or write from the same table DO end up causing MySQL to understand there is a conflict and one of the threads will block which is what we need.

So I do think that this implementation is required by ALL MySQL users, not just those using a cluster.