InMemory saga persister creates duplicate saga instances #4182
Conversation
danielmarbach
changed the title
| } | ||
| } | ||
|
|
||
| [Serializable] |
There was a problem hiding this comment.
Nitpick: We don't need the [Serializable]
| public string SomeId { get; set; } | ||
| } | ||
|
|
||
| [Serializable] |
There was a problem hiding this comment.
Nitpick: We don't need the [Serializable]
| public string SomeId { get; set; } | ||
| } | ||
|
|
||
| [Serializable] |
There was a problem hiding this comment.
Nitpick: We don't need the [Serializable]
There was a problem hiding this comment.
Remember this is v5. It uses it consistently
There was a problem hiding this comment.
Not a 100% https://github.com/Particular/NServiceBus/blob/master/src/NServiceBus.AcceptanceTests/Audit/When_a_replymessage_is_audited.cs#L112 and there's probably more. But as I said nitpick, I'll make sure we remove them for v6.1
|
@Particular/nservicebus-maintainers please review. In order to keep it consistent with v5 behavior I'm calling |
V6 won't let you change the correlation property. I'd say we leave as is for v5? |
| where | ||
| (s.Value.SagaEntity.GetType() == saga.GetType() && (s.Key != saga.Id)) | ||
| select s.Value; | ||
| if (sagaMetaData.CorrelationProperties.Count == 0) return; |
There was a problem hiding this comment.
let's avoid the if short from
| { | ||
| if (storedSaga.Value.SagaData.GetType() != sagaType || (storedSaga.Key == saga.Id)) continue; |
There was a problem hiding this comment.
let's avoid the if short from
There was a problem hiding this comment.
It was used in other parts in this file as well that's why I kept it consistent.
There was a problem hiding this comment.
not considering this as a blocker, but I don't think consistency on this brings any value in that case.
There was a problem hiding this comment.
of course it does, at least in the same file the same consistent code style should be used. When I port back into existing code I try to stay consistent with what is there from a style perspective. Applying new formatting rules doesn't make much sense either.
There was a problem hiding this comment.
If you feel strongly about it @timbussmann feel free to apply your desired style changes while merging
There was a problem hiding this comment.
@timbussmann I applied formatting as a separate commit just for you ❤️
|
@timbussmann please merge if you're happy with the changes |
Who's affected
All users using InMemory saga persister that have messages concurrently trying to start the same saga instance on an endpoint with concurrency set to a value above the default of 1.
Symptoms
Two saga instances are created which leads to incorrect business behavior.
Solution
v5 supports multiple correlation properties. So here the lock will be taken on the
FullNameof the saga entity type