1. Community
  2. Plugin Support
  3. True PVE
Bug in AddOrUpdateMapping - DynamicPVP relatedSolved
Plugin Page
Posted 5 years ago
#1

Hi Nivex,

I've been chasing the error where when using TruePVE schedules and DynamicPVP together that the scheduler stops working. I mean it's been a LOOOOOOOOONG time we've been looking for this. You fixed the NREs by ignoring the null outputs in ClockUpdate but that didn't fix the underlying issue. I had some time tonight and dug in finally.

It turns out that this is related to a "fix" RFC put in before you took this over. To quote from the changelog "Added first pass fix for AddOrUpdateMapping() clearing all but the default ruleset. Probably more work to be done." He added LoadConfiguration() to the top of AddOrUpdateMapping and this resets data.schedule.parsedEntries. specifically this line of code from LoadConfiguration resets the list:

data = Config.ReadObject<TruePVEData>() ?? null;​

Data is being overwritten like the plugin just loaded. Schedule.Init() only ever gets called when a new TruePVEData object is created. This only happens when the plugin loads. So calling LoadConfiguration will reset anything that is initialized in data when the plugin loads. 

I can reproduce this error independantly of DynamicPVP by simply calling AddOrUpdateMapping from anywhere. After I do so the scheduler stops working and parsedEntries is empty. 

I'm surprised more people haven't had issues with other plugins that call this API. I guess not many are using the scheduler. I dont see what value LoadConfiguration adds in that procedure so I've commeted it out in my version and so far no ill effects. 

You might also consider getting rid of the parser and just have the parsedEntities broken into nice little JSON nuggets. Not as pretty as a one liner but easier to code.

I hope this helps you fix this very very VERY old issue. Sorry it took me so long man...

Cheers,
-Fire
Posted 5 years ago
Last updated 5 years ago
#2

I knew it wasn't my code, but people insisted over and over again that it was. I looked for hours at the same 10 lines of code changes that I made, and each time came to the conclusion that it was impossible. Glad I wasn't losing my fkn mind. Great find!

issues with schedules are still common and I imagine this is why. so what is the solution then? Remove the LoadConfiguration call?

Merged post

I will apply this fix in the next update!

Posted 5 years ago
#3

Hey Nivex,

Yeah removing LoadConfiguration() from AddOrUpdateMapping should fix the issue.

I'm not sure what the original intent of adding it was or what bug they were trying to fix but it seems to be unnecessary. The only scenario I can think that this would help would be if someone modifies the config externally and the API was called before the plugin could be reloaded then any changes they made would be erased from the config when SaveData was called. And this edge case does exist. I've hit it myself from time to time. I think the intent was to come back and make it better later and later never happened.

So the simple fix for now is to remove LoadConfiguration from AddOrUpdateMapping but the long term goal might be to do something similar but ensure the schedule.init is run again....or like I suggested remove the scheduler translator all together and go for simple json nuggets to define the schedules because it's only the memory resident variables that are being lost, not config settings.

All the try/catches in ClockUpdate need to be replaced with some more robust simplified logic as well. I've never seen a try/catch used quite like that before where if/then/else would have worked just as well or better. If I get some free cycles I'll throw something together for you to review and see if you like.

Take it easy
-Fire

Posted 5 years ago
Last updated 5 years ago
#4

would a possible solution to this edge case be to change AddOrUpdateMapping to return an integer instead? -1 = not initialized, 0 = rejected, 1 = success?

I don't know what you mean by nuggets or how to do that. I think your terminology is lost on me. Please clarify in clear words :P

I've seen the mess that is ClockUpdate and I'd rather not tinker with it right now :p

For the time being I will remove the LoadConfiguration call in order to fix this outstanding bug. Any edge cases can be resolved later

Locked automatically