shirsig/Mail

Latest commit causes an infinite loop/app hang

Closed this issue · 26 comments

Latest commit causes an infinite loop/app hang

can you be more specific? nothing of the sort happening for me

Well I can only comment on what I'm seeing, as soon as I open the mailbox interface (interacting with a mailbox) the entire game hangs and doesn't recover. Downgrading to the previous commit fixed this issue for me.

addon conflict perhaps?

I got this issue with the newest postal version and downgraded again,
Happened with a mailbox with many mails inside the mailbox. After opening mailbox icons are not displayed and game hangs.

Addons:
_LazyPig: enabled
_LazyPigMultibox: disabled
AtlasLoot: enabled
AttackBar: enabled
AutoBar: disabled
aux-addon: enabled
Banana: disabled
BetterCharacterStats: enabled
BigWigs: enabled
BuyPoisons: enabled
CThunWarner: disabled
CallToArms: disabled
Cartographer: enabled
Cartographer_Herbalism: enabled
Cartographer_Mining: disabled
CastModifier: enabled
ChatSuey: enabled
ChatSuey-ItemLinkHover: enabled
ChatSuey-Mentions: enabled
Chronometer: enabled
ClassicSnowFall: enabled
Clean_Up: enabled
crafty: enabled
DebuffTimers: disabled
DPSMate: disabled
EnemyFrames: enabled
EQL3: disabled
HealComm: enabled
!ImprovedErrorFrame: disabled
KLHThreatMeter: enabled
KTMAutoHider: disabled
MikScrollingBattleText: enabled
MikScrollingBattleTextOptions: enabled
MinimapButtonFrame: disabled
MobInfo2: disabled
ModifiedPowerAuras: enabled
CEnemyCastBar: disabled
CECB_Options: disabled
npcscan: disabled
Outfitter: enabled
Postal: enabled
QuickHeal: disabled
RingMenu: enabled
SimpleCombatLog: disabled
SmartBuff: disabled
StunWatch: enabled
TrinketMenu: enabled
vQueue: disabled
WeaponQuickSwap: enabled
WeaponRebuff: enabled
WIM: disabled
Zorlen: disabled
ClassicDB: disabled
pfUI: enabled
ShaguInventory: enabled
!Questie: disabled
VCB: enabled
WHDB: disabled
ShaguDB: disabled
ShaguQuest: disabled
MetaMap: disabled
MetaMapBWP: disabled
MetaMapWKB: disabled
SUCC-ecb: disabled
PallyPower: disabled
Decursive: disabled
YaHT: disabled
MobHealth: enabled
Talentsaver: enabled
BuffReminder: enabled
cooline: enabled
SW_Stats: enabled
BigWigs_AQ40: enabled
BigWigs_BWL: enabled
BigWigs_MC: enabled
BigWigs_Naxxramas: enabled
BigWigs_Onyxia: enabled
BigWigs_Other: enabled
BigWigs_AQ20: enabled
BigWigs_ZG: enabled
BigWigs_CThunAssist: enabled
BigWigs_CommonAuras: enabled
BigWigs_LoathebTactical: enabled
BigWigs_ThaddiusArrows: enabled
BigWigs_RazuviousAssistant: enabled
SW_FixLogStrings: enabled
MBB: enabled
Samuel: disabled
SP_SwingTimer: disabled
RABuffs: disabled
RAT: enabled
VF_RaidDamage: enabled
VF_RealmPlayers: enabled
TargetAssist: enabled
cdframes: enabled
ShaguPlates: disabled
ccwatch: enabled
IMBA: disabled
MetaMapBLT: disabled
MetaMapBKP: disabled
MetaMapEXP: disabled
MetaMapCVT: disabled
MetaMapFWM: disabled
MetaMapNBK: disabled
MetaMapHLP: disabled
MetaMapQST: disabled
pfQuest: enabled
VanillaGuide: disabled
MetaMapZSM: disabled
ActionBarSaver: disabled
FlightMap: enabled
HonorSpy: enabled
Atlas: disabled
AtlasQuest: disabled
AfterCast: disabled
Alkitron_HonorTab: disabled
Antagonist: enabled
BearCastBar: disabled

I tried it with a full mailbox and no noticeable performance drop. can you try to find out which addon is causing it by selectively enabling them?

I'm suspecting pfUI, but have not confirmed it yet

I have now disabled every addon except Mail and the issue persists.

I also get this error:

https://github.com/shirsig/Mail/blob/master/Mail.lua#L126-L129 Seems to be the issue, commenting it fixes it. The problem seems to lie in the GetInboxHeaderInfo(...) function.

It seems that having many uniterated new mail in there (say, several pages of auctions expiring) causes too much iteration which uses up all the available memory and takes way too long to process).

For everyone else reading this, you can replace the corresponding function inside Mail.lua with

function MAIL_INBOX_UPDATE()
	--for i = 1, GetInboxNumItems() do
	--	local _, _, sender, _, _, _, _, _, _, _, _, canReply = GetInboxHeaderInfo(i)
	--	if sender and canReply then
	--		addAutoCompleteName(sender)
	--	end
	--end
end

It fixes the issue without losing any significant functionality.

I know, this was the only change pretty much, but I don't understand how that's possible. You can only have 50 mails in your inbox afaik at a time, more are only loaded once some of those are looted. I tried it with a full inbox and there was no noticeable impact at all. Seems especially strange how it impacts the memory usage at startup. The only way this is possible is if it has large savedvariables, like tens of thousands of table entries, but I doubt you got mail from tens of thousands of distinct senders in the last 30 days? (edit: that was actually from the other issue but I would assume they're related)

Mail_AutoCompleteNames[key][name] = GetTime()

I'll try to let some auctions expire and see what happens, only tried it with regular mail so far.

auction expired mail also causes no problems for me

Well I disabled all addons except this one and the issue still exists.

Also note that it|s not the table entry that seems to cause the issue, but rather the GetInboxHeaderInfo(i) function.

I should also note that while the crash doesn't happe anymore with this "fix", there is still significant lag on opening the box for the first time after logging in.

That can only be because of savedvariables. You did try to delete them at least once?

I did a quick check on the filesizes but they were all small relating to postal/Mail

Is it possible there is a cross interference with aux? My aux file is quite large.

Doubtful, I'm using aux too. Maybe try
/run Mail_AutoCompleteNames = {}
/run ReloadUI()
That's the only place where anything could be stored anyway.

I can see process memory shooting up crazy when i open the mailbox, this is a much deeper issue than it looks.

Running the commands had no effect.

I think I found the culprit. It seems that the MAIL_INBOX_UPDATE function is called an absurd amount of times (40 times, getting progressively worse for each page), each of them running through each mail again. This results in thousands of loop iterations which explains why the whole game just stops working entirely.

I hooked a counter variable to the update function just to see how high it counts and after stepping through all pages of my mailbox, this is what I came up with:

So this is something in the order of 150,000 loop iterations on a bad day. This seems to mostly happen on logging in and checking for the first time. Subsequent openings of the mailbox do not trigger an unusual amount of update events.

Strange that this is not happening for me though. This could be simply solved by throttling the function but I'd rather know why it's happening before doing some hackfix.

Do you have a special client in some way?

Nope, no special client. I use VF_Launcher though, which does wipe the WDB everytime you log in, this may affect it, but I'm not sure why it would.

I have a theory though: For some reason, the names of all senders show up as "Unknown" initially, this is also where the crash occurs, so what if the script triggers the MAIL_INBOX_UPDATE event everytime it resolves one of those names?

maybe if each one triggers it countless times, otherwise it wouldn't be nearly enough. What I suspect might cause the freezing is when GetInboxHeaderInfo causes a new event (which are handled synchronously) and then recursively keeps triggering them. Could you try this and see if it solves it?:

local active = false
function MAIL_INBOX_UPDATE()
	if not active then
		active = true
		for i = 1, GetInboxNumItems() do
			local _, _, sender, _, _, _, _, _, _, _, _, canReply = GetInboxHeaderInfo(i)
			if sender and canReply then
				addAutoCompleteName(sender)
			end
		end
		active = false
	end
end

I tried it twice, it does not solve the issue.

I resorted to changing the event creation and remove the call to MAIL_INBOX_UPDATE for now

for _, event in {'ADDON_LOADED', 'PLAYER_LOGIN', 'UI_ERROR_MESSAGE', 'CURSOR_UPDATE', 'BAG_UPDATE', 'MAIL_CLOSED', 'MAIL_SEND_SUCCESS', 'MAIL_INBOX_UPDATE'} do

alright, well, I just changed the implementation for getting sender names from listening to this event to hooking GetInboxHeaderInfo. Should still get all names and avoid this issue completely.