[dbal] Use concurrent fetch message approach (no transaction, no pessimistic lock) #613
Conversation
pkg/dbal/DbalConsumerHelperTrait.php
Outdated
| $this->removeExpiredMessagesLastExecutedAt = microtime(true); | ||
| } | ||
|
|
||
| if ((microtime(true) - $this->removeExpiredMessagesLastExecutedAt) < 1) { |
There was a problem hiding this comment.
you could put that in an elseif so it's not executed needlessly on the first execution
| ; | ||
|
|
||
| if (false == $deliveredMessage) { | ||
| throw new \LogicException('There must be a message at all times at this stage but there is no a message.'); |
There was a problem hiding this comment.
this is not certain. it could be deleted manually or some other concurrent process between the update and the select. unlikely but possible. why not just return null?
There was a problem hiding this comment.
This should not happen. Few lines above the message was updated with deliveryId (which is unique and only known by this consumer) therefor if we cannot select it few lines later something went wrong.
There was a problem hiding this comment.
When I truncate the table, this is gone. And the consumer would fail for no reason.
There was a problem hiding this comment.
Why keep the exception? This is not race-condition safe.
There was a problem hiding this comment.
When I truncate the table, this is gone. And the consumer would fail for no reason.
It would fail, exit and restarted by supervisord or any other process manager.
If we remove the exception we might have a bug or something still would not know about it cuz it would silently continue fetching new messages without a warning.
Truncating a table while there are consumers working on it is not a valid way to do. I could imagine many cases which would go wrong in this case (for example a message has been processed and a consumer tries to acknowledge it but it has gone).
There was a problem hiding this comment.
Before it used a write lock and thus it protects against concurrent access causing race conditions. Now this is gone, so you need to make your consumers handle that gracefully. Throwing an exception and relying on the consumer to be restarted is just a workaround for a buggy consumer.
|
The new approach looks ok. Btw, are you sure the old locking approach only locked the first row of the ordered query? Seems questionable considering https://bugs.mysql.com/bug.php?id=67745 and https://hoopercharles.wordpress.com/2011/11/21/select-for-update-in-what-order-are-the-rows-locked/ |
I've not been aware of the issue. There is always a room for some surprises. Thank you for pointing out. The issue comes in only if there is a filesort takes place, right? When the first version (based on pessimistic lock) was released we performed tests to check that it used indexes. We've not run the tests lately.
Is this feature a part of SQL standard? It is supported by MySQL since version 8 only and I am not sure Doctrine DBAL provides an API for using it. I'll look into it. |
pkg/dbal/DbalConsumerHelperTrait.php
Outdated
| $this->getConnection()->createQueryBuilder() | ||
| ->delete($this->getContext()->getTableName()) | ||
| ->andWhere('(time_to_live IS NOT NULL) AND (time_to_live < :now)') | ||
| ->andWhere('(redelivered = false OR delivery_id IS NULL)') | ||
| ->setParameter(':now', (int) time(), Type::BIGINT) | ||
| ->setParameter('redelivered', false, Type::BOOLEAN) |
pkg/dbal/DbalConsumerHelperTrait.php
Outdated
|
|
||
| while (microtime() < $endAt) { |
There was a problem hiding this comment.
you are comparing string with float here. maybe you meant to use microtime(true)?
The consumption logic could be made even simpler and at the same time more performant (still to be proved) cuz there would be fewer round trips, fewer locks.
fixes #605