From 7118b1bc5631be3690107b8a700f369f24e369d7 Mon Sep 17 00:00:00 2001 From: Cyril Soler Date: Fri, 20 Sep 2024 18:54:16 +0200 Subject: [PATCH 1/3] added method to check/remove duplicate message ids, and improved the generation of new unique message ids --- src/services/p3msgservice.cc | 174 ++++++++++++++++++++++++++++++++--- src/services/p3msgservice.h | 5 +- 2 files changed, 165 insertions(+), 14 deletions(-) diff --git a/src/services/p3msgservice.cc b/src/services/p3msgservice.cc index c8b19f126..25c277774 100644 --- a/src/services/p3msgservice.cc +++ b/src/services/p3msgservice.cc @@ -119,7 +119,7 @@ p3MsgService::p3MsgService( p3ServiceControl *sc, p3IdService *id_serv, p3GxsTrans& gxsMS ) : p3Service(), p3Config(), gxsOngoingMutex("p3MsgService Gxs Outgoing Mutex"), mIdService(id_serv), - mServiceCtrl(sc), mMsgMtx("p3MsgService"), mMsgUniqueId(0), + mServiceCtrl(sc), mMsgMtx("p3MsgService"), recentlyReceivedMutex("p3MsgService recently received hash mutex"), mGxsTransServ(gxsMS) { @@ -130,7 +130,6 @@ p3MsgService::p3MsgService( p3ServiceControl *sc, p3IdService *id_serv, /* MsgIds are not transmitted, but only used locally as a storage index. * As such, thay do not need to be different at friends nodes. */ - mMsgUniqueId = 1; mShouldEnableDistantMessaging = true; mDistantMessagingEnabled = false; @@ -171,7 +170,13 @@ p3MsgService::~p3MsgService() uint32_t p3MsgService::getNewUniqueMsgId() { RS_STACK_MUTEX(mMsgMtx); /********** STACK LOCKED MTX ******/ - return mMsgUniqueId++; + + uint32_t res; + + do { res = RsRandom::random_u32(); } while(mAllMessageIds.find(res)!= mAllMessageIds.end()); + + mAllMessageIds.insert(res); + return res; } int p3MsgService::tick() @@ -647,8 +652,6 @@ bool p3MsgService::parseList_backwardCompatibility(std::list& load) auto msi = new RsMailStorageItem(); msi->msg = *mitem; msg_map[mitem->msgId] = msi; - - mMsgUniqueId = std::max(mMsgUniqueId,mitem->msgId+1); } else if(nullptr != (mti = dynamic_cast(it))) msg_tags.push_back(mti); @@ -789,7 +792,9 @@ bool p3MsgService::parseList_backwardCompatibility(std::list& load) bool p3MsgService::loadList(std::list& load) { - auto gxsmIt = load.begin(); + RS_STACK_MUTEX(mMsgMtx); // lock ere, because we need to load, then check for duplicates, and this needs to be done in the same lock. + + auto gxsmIt = load.begin(); RsMsgGRouterMap* gxsmailmap = dynamic_cast(*gxsmIt); if(gxsmailmap) { @@ -801,7 +806,6 @@ bool p3MsgService::loadList(std::list& load) } std::list unhandled_items; - uint32_t max_msg_id = 0 ; // load items and calculate next unique msgId for(auto it = load.begin(); it != load.end(); ++it) @@ -887,13 +891,9 @@ bool p3MsgService::loadList(std::list& load) { RsErr() << "Loaded msg with msg.to=" << msi->to ; - if(msi->msg.msgId > max_msg_id) - max_msg_id = msi->msg.msgId ; - /* STORE MsgID */ if (msi->msg.msgId != 0) { - RS_STACK_MUTEX(mMsgMtx); /* switch depending on the PENDING * flags @@ -923,7 +923,6 @@ bool p3MsgService::loadList(std::list& load) else unhandled_items.push_back(*it); } - mMsgUniqueId = max_msg_id+1; parseList_backwardCompatibility(unhandled_items); @@ -934,9 +933,159 @@ bool p3MsgService::loadList(std::list& load) load.clear(); +#ifdef MSG_DEBUG + // list all the msg Ids + auto print_msgids = [](const std::map& mp,const std::string& name) { + std::cerr << "Message ids in box " << name << " : " << std::endl; + for(auto it:mp) + std::cerr << " " << it.first << " " << it.second->msg.msgId << std::endl; + }; + print_msgids(mSentMessages,"Sent"); + print_msgids(mTrashMessages,"Trash"); + print_msgids(mDraftMessages,"Drafts"); + print_msgids(mReceivedMessages,"Received"); + + std::cerr << "Outgoing messages: " << std::endl; + for(auto m:msgOutgoing) + { + std::cerr << " parent " << m.first << " : " << std::endl; + for(auto p:m.second) + std::cerr << " " << p.first << std::endl; + } +#endif + + // This was added on Sept 20, 2024. It is here to fix errors following a bug that caused duplication of + // some message ids. This should be kept because it also creates the list that is stored in mAllMessageIds, + // that is further used by getNewUniqueId() to create a unique message Id. + + locked_checkForDuplicates(); return true; } +// Two generic methods to replace elements in a map, when the first (resp. second) element matches an id to substitute. + +template void replace_first(std::map& mp,uint32_t old_id,uint32_t new_id) +{ + auto tt = mp.find(old_id); + if(tt == mp.end()) + return; + + auto sec = tt->second; + mp.erase(tt); + mp[new_id] = sec; +} + +template void replace_second(std::map& mp,uint32_t old_id,uint32_t new_id) +{ + for(auto& it:mp) + if(it.second == old_id) + it.second = new_id; +} +void p3MsgService::locked_checkForDuplicates() +{ + bool changed = false; + std::set already_known_ids; + + auto replace_parent = [](std::map& mp,uint32_t old_id,uint32_t new_id) + { + for(auto& it:mp) + if(it.second->parentId == old_id) + { + RsWarn() << "Replacing parent ID " << old_id << " of message " << it.first << " with new parent " << new_id << std::endl; + it.second->parentId = new_id; + } + }; + + auto check = [&already_known_ids,&changed,this,replace_parent](std::map& mp,const std::string& name) + { + std::map new_mp; + + for(std::map::iterator it(mp.begin());it!=mp.end();) + { + if(already_known_ids.find(it->first)!=already_known_ids.end()) + { + changed=true; + + // generate a new ID + uint32_t old_id = it->first; + uint32_t new_id; + do { new_id = RsRandom::random_u32() ; } while(already_known_ids.find(new_id)!=already_known_ids.end()); + + RsWarn() << "Duplicate ID " << it->first << " found in message box " << name << ". Will be replaced by new ID " << new_id << std::endl; + + // replace the old ID by the new, everywhere + + // 1 - in the map itself + + it->second->msg.msgId = new_id; + new_mp[new_id] = it->second; // put the modified item in a new map, so as not to have the same item visited twice in this loop. + + auto tmp = it; // remove the item from the map + tmp++; + mp.erase(it); + it = tmp; + + // 2 - everywhere it is designated as parent + + replace_parent(mTrashMessages, old_id,new_id); + replace_parent(mSentMessages, old_id,new_id); + replace_parent(mDraftMessages, old_id,new_id); + replace_parent(mReceivedMessages,old_id,new_id); + + // 3 - mMsgOutgoing refers to original msg in Sent, so the substitution must happen there too + + replace_first(msgOutgoing,old_id,new_id); + + // 4 - in GRouter and GxsTrans correspondance maps, and recently received messages + + replace_second(_grouter_ongoing_messages ,old_id,new_id); + replace_second(gxsOngoingMessages ,old_id,new_id); + replace_second(mRecentlyReceivedMessageHashes,old_id,new_id); + + // 6 - in mParentId correspondance map + + replace_first(mParentId,old_id,new_id); + } + else + ++it; + + already_known_ids.insert(it->first); + } + mp.insert(new_mp.begin(),new_mp.end()); // merge back the new list in the modified one + }; + + check(mTrashMessages,"mTrashMessages"); + check(mSentMessages,"mSentMessages"); + check(mDraftMessages,"mDraftMessages"); + check(mReceivedMessages,"mReceivedMessages"); + + // now check msgOutgoing. The first element refers to an element in mSentMessages, so it's already been treated + + for(auto& it:msgOutgoing) + { + std::map to_switch; + + for(auto sit:it.second) + if(already_known_ids.find(sit.first) != already_known_ids.end()) + { + uint32_t new_id; + do { new_id = RsRandom::random_u32() ; } while(already_known_ids.find(new_id)!=already_known_ids.end()); + + to_switch[sit.first] = new_id; + changed=true; + } + else + already_known_ids.insert(sit.first); + + for(auto sit:to_switch) + replace_first(it.second,sit.first,sit.second); + } + + mAllMessageIds = already_known_ids; + + if(changed) + IndicateConfigChanged(RsConfigMgr::CheckPriority::SAVE_NOW); +} void p3MsgService::loadWelcomeMsg() { /* Load Welcome Message */ @@ -2698,7 +2847,6 @@ RsMsgItem *p3MsgService::createOutgoingMessageItem(const RsMailStorageItem& msi, void p3MsgService::debug_dump() { std::cerr << "Dump of p3MsgService data:" << std::endl; - std::cerr << " mMsgUniqueId: " << mMsgUniqueId << std::endl; auto display_box = [=](const std::map& msgs,const std::string& box_name) { std::cerr << " " + box_name + ":" << std::endl; for(auto msg:msgs) diff --git a/src/services/p3msgservice.h b/src/services/p3msgservice.h index 0e3a7ff33..f3a30050e 100644 --- a/src/services/p3msgservice.h +++ b/src/services/p3msgservice.h @@ -158,6 +158,7 @@ class p3MsgService : private: void locked_sendDistantMsgItem(RsMsgItem *msgitem, const RsGxsId &from, uint32_t msgId); bool locked_getMessageTag(const std::string &msgId, Rs::Msgs::MsgTagInfo& info); + void locked_checkForDuplicates(); RsMailStorageItem *locked_getMessageData(uint32_t mid) const; /** This contains the ongoing tunnel handling contacts. @@ -242,7 +243,9 @@ class p3MsgService : std::map mTags; - uint32_t mMsgUniqueId; + // Set of messages ids used. Any new msg id generated will be checked against this set and added to it. + std::set mAllMessageIds; + std::map mRecentlyReceivedMessageHashes; RsMutex recentlyReceivedMutex; From afdefd34844068f3f33034299d654938da49e2eb Mon Sep 17 00:00:00 2001 From: Cyril Soler Date: Mon, 23 Sep 2024 10:34:43 +0200 Subject: [PATCH 2/3] fixed missing update of already_known_ids --- src/services/p3msgservice.cc | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/services/p3msgservice.cc b/src/services/p3msgservice.cc index 25c277774..4f7ec2cf4 100644 --- a/src/services/p3msgservice.cc +++ b/src/services/p3msgservice.cc @@ -956,7 +956,7 @@ bool p3MsgService::loadList(std::list& load) // This was added on Sept 20, 2024. It is here to fix errors following a bug that caused duplication of // some message ids. This should be kept because it also creates the list that is stored in mAllMessageIds, - // that is further used by getNewUniqueId() to create a unique message Id. + // that is further used by getNewUniqueId() to create unique message Ids in a more robust way than before. locked_checkForDuplicates(); return true; @@ -1011,6 +1011,8 @@ void p3MsgService::locked_checkForDuplicates() uint32_t new_id; do { new_id = RsRandom::random_u32() ; } while(already_known_ids.find(new_id)!=already_known_ids.end()); + already_known_ids.insert(new_id); + RsWarn() << "Duplicate ID " << it->first << " found in message box " << name << ". Will be replaced by new ID " << new_id << std::endl; // replace the old ID by the new, everywhere @@ -1025,6 +1027,9 @@ void p3MsgService::locked_checkForDuplicates() mp.erase(it); it = tmp; + // Next, we replace the old id by the new, everywhere it is mentionned. Of course this may not be correct, since + // the actual old id may be mentionned on purpose. Still, there is absolutely no way to know which is the right one. + // 2 - everywhere it is designated as parent replace_parent(mTrashMessages, old_id,new_id); @@ -1073,6 +1078,7 @@ void p3MsgService::locked_checkForDuplicates() to_switch[sit.first] = new_id; changed=true; + already_known_ids.insert(new_id); } else already_known_ids.insert(sit.first); From 3b38f1b6063e237663b91750630d28ea2447f525 Mon Sep 17 00:00:00 2001 From: Cyril Soler Date: Mon, 23 Sep 2024 22:02:23 +0200 Subject: [PATCH 3/3] added notification on change --- src/services/p3msgservice.cc | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/src/services/p3msgservice.cc b/src/services/p3msgservice.cc index 4f7ec2cf4..aee0a76e1 100644 --- a/src/services/p3msgservice.cc +++ b/src/services/p3msgservice.cc @@ -983,8 +983,8 @@ template void replace_second(std::map& mp,uint32_t old_id,u } void p3MsgService::locked_checkForDuplicates() { - bool changed = false; std::set already_known_ids; + std::set changed_msg_ids; auto replace_parent = [](std::map& mp,uint32_t old_id,uint32_t new_id) { @@ -996,7 +996,7 @@ void p3MsgService::locked_checkForDuplicates() } }; - auto check = [&already_known_ids,&changed,this,replace_parent](std::map& mp,const std::string& name) + auto check = [&already_known_ids,&changed_msg_ids,this,replace_parent](std::map& mp,const std::string& name) { std::map new_mp; @@ -1004,14 +1004,13 @@ void p3MsgService::locked_checkForDuplicates() { if(already_known_ids.find(it->first)!=already_known_ids.end()) { - changed=true; - // generate a new ID uint32_t old_id = it->first; uint32_t new_id; do { new_id = RsRandom::random_u32() ; } while(already_known_ids.find(new_id)!=already_known_ids.end()); already_known_ids.insert(new_id); + changed_msg_ids.insert(std::to_string(new_id)); RsWarn() << "Duplicate ID " << it->first << " found in message box " << name << ". Will be replaced by new ID " << new_id << std::endl; @@ -1076,8 +1075,10 @@ void p3MsgService::locked_checkForDuplicates() uint32_t new_id; do { new_id = RsRandom::random_u32() ; } while(already_known_ids.find(new_id)!=already_known_ids.end()); + RsWarn() << "Duplicate ID " << sit.first << " found in msgOutgoing. Will be replaced by new ID " << new_id << std::endl; + to_switch[sit.first] = new_id; - changed=true; + changed_msg_ids.insert(std::to_string(new_id)); already_known_ids.insert(new_id); } else @@ -1089,8 +1090,15 @@ void p3MsgService::locked_checkForDuplicates() mAllMessageIds = already_known_ids; - if(changed) + if(!changed_msg_ids.empty()) + { IndicateConfigChanged(RsConfigMgr::CheckPriority::SAVE_NOW); + + auto pEvent = std::make_shared(); + pEvent->mMailStatusEventCode = RsMailStatusEventCode::MESSAGE_CHANGED; + pEvent->mChangedMsgIds = changed_msg_ids; + rsEvents->postEvent(pEvent); + } } void p3MsgService::loadWelcomeMsg() {