-
Notifications
You must be signed in to change notification settings - Fork 578
Description
From Andreas Meisner on the mailing list:
Hi all,
this is my first email on this list and I am not yet familiar with the
rules here.
I have found the following issue, because I have implemented a
IMessageStore for MSSQL server (using EF) having indices on the database
tables:
In class Session we have the method Persist(Message, string):
protected void Persist(Message message, string messageString)
{
if (PersistMessages)
{
SeqNumType msgSeqNum =
message.Header.GetULong(Fields.Tags.MsgSeqNum);
_state.Set(msgSeqNum, messageString);
}
_state.IncrNextSenderMsgSeqNum();
}
The call of Set() and IncrNextSenderMsgSeqNum() should be in one
transaction, but currently I have a straight forward implementation with
no dependencies between these methods. In my case a call to Set() was
successful, but the call to IncrNextSenderMsgSeqNum() failed, leaving
the database in an inconsitant state. I think the same could happen when
using FileStore, but here we do not have an index that could cause an
exception.
I can catch this in my DbMessageStore implementation, but it would be
nicer to have only one method in this case.
So my proposal for a change is:
- Add a new method in interface IMessageStore with a default
implementation:
public interface IMessageStore : IDisposable
{
// ... existing members
public bool SetAndIncrNextSenderMsgSeqNum(SeqNumType msgSeqNum,
string msg)
{
bool result = Set(msgSeqNum, msg);
IncrNextSenderMsgSeqNum();
return result;
}
}
- Add the method with the same signature to class SessionState:
public bool SetAndIncrNextSenderMsgSeqNum(SeqNumType msgSeqNum,
string msg)
{
lock (_sync) { return
MessageStore.SetAndIncrNextSenderMsgSeqNum(msgSeqNum, msg); }
}
- Call the new method in class Session:
protected void Persist(Message message, string messageString)
{
if (PersistMessages)
{
SeqNumType msgSeqNum =
message.Header.GetULong(Fields.Tags.MsgSeqNum);
_state.SetAndIncrNextSenderMsgSeqNum(msgSeqNum,
messageString);
}
else
{
_state.IncrNextSenderMsgSeqNum();
}
}
This change should be 99.9% backwards compatible and should not affect
any existing implementation. The only difference (the 0.1%) is that the
call to Set() and IncrNextSenderMsgSeqNum() is in one lock(_sync) in
class SessionState, but this should be also an improvement.
Please let me know, if I can add a new pull request with these changes.
Thanks,
Andreas