Command Reward to load default quests throws an IllegalArgumentException

[luke-bravenboer]

Firstly I'd like to be clear that this isn't actually causing a crash, just throwing an exception in a separate thread (just figured you should know regardless). I wouldn't have even noticed it if it weren't for VanillaFix's fantastic exception-catching toast notifications.

Having a Console Command reward execute the command '/bq_admin default load' spits out the following exception to the logs:

java.util.concurrent.ExecutionException: java.lang.IllegalArgumentException: ID cannot be negative
    at java.util.concurrent.FutureTask.report(FutureTask.java:122) ~[?:1.8.0_51]
    at java.util.concurrent.FutureTask.get(FutureTask.java:192) ~[?:1.8.0_51]
    at net.minecraft.util.Util.func_181617_a(SourceFile:47) [h.class:?]
    at net.minecraft.client.Minecraft.func_71411_J(Minecraft.java:1086) [bib.class:?]
    at net.minecraft.client.Minecraft.func_99999_d(Minecraft.java:397) [bib.class:?]
    at net.minecraft.client.main.Main.main(SourceFile:123) [Main.class:?]
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~[?:1.8.0_51]
    ...
Caused by: java.lang.IllegalArgumentException: ID cannot be negative
    at betterquesting.api2.storage.BigDatabase.add(BigDatabase.java:69) ~[BigDatabase.class:?]
    at betterquesting.network.handlers.PktHandlerQuestSync.handleClient(PktHandlerQuestSync.java:36) ~[PktHandlerQuestSync.class:?]
    at betterquesting.network.PacketQuesting$HandleClient$1.run(PacketQuesting.java:124) ~[PacketQuesting$HandleClient$1.class:?]
    at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511) ~[?:1.8.0_51]
    at java.util.concurrent.FutureTask.run(FutureTask.java:266) ~[?:1.8.0_51]
    at net.minecraft.util.Util.func_181617_a(SourceFile:46) ~[h.class:?]
    ... 17 more

The command still executes & the default quests do get reloaded successfully; so there's no visible impact at all for the user. It's only an issue for me when the aforementioned VanillaFix mod is also installed, as it sends a toast notification every time this happens.

Versions:

BetterQuesting-3.5.262 StandardExpansion-3.4.143 Forge: 14.23.4.2705

[Funwayguy]

To be blunt, this isn't so much a bug as it is just a REALLY BAD IDEA. Just because the command reward can run commands that edits its own quest lines doesn't mean it should. You are effectively deleting then loading a new database of quests from within the old copy while it is still being processed . This isn't something I intend to support for, I'd hope, obvious reasons.

As a side note, I would not recommend trying to reload that world, or any other, without restarting the client fully. This error may have put the quest database in a undefined state and trying to reload a world may result in old data being mashed together with the new and then saved over when you back out.

[luke-bravenboer]

Whoa, I apologize! I thought it was a pretty common thing for modpacks to do! ._.

In all seriousness though, when modpacks update & quests are fixed or added, this was a convenient way to allow players to actually receive the new quests without having to have cheats enabled. Is there a more supported way to allow existing players to receive quest updates, or do we literally just have to teach 'em the command & how to cheat?

No worries about the side note, I keep my modpack configs checked in to source control ;)

[Funwayguy]

You don't have to apologize. I was more being blunt to drive home the point that this could actually do damage to your quests if they are still in development.

As far as I'm aware, most pack devs leave it up to the users to run the command themselves when they're ready. There was a mod that automatically did this for you (can't remember what it was) but it had a habit of running it too often which had some side effects in BQ2 such as sometimes just loading a blank database. That said, the new database system I've written BQ3 should be robust enough to not have runaway data like that. If you can find the mod then sure, you can use that, otherwise I'd just leave it to the users to type the command.

[luke-bravenboer]

Fair enough! That's a shame, but it is what it is. It would be nice to get an officially-supported way to update quests (something like a blinking "!" notification on the main menu that users can click to refresh their quest database, perhaps?).

Thanks for the explanation though, had no idea that there were concurrency considerations on the table- its pretty clearly dangerous when you put it like that. Out of curiousity, and I know I'm just looking at this naively, but- what's so fundamentally bad about self-modifying quests? Wouldn't you want to separate the behaviour so that it wouldn't even be able to get into the situation of modifying the quest database while it's being processed? I've played a few modpacks that had such a repeatable console command quest to reload the quest database & never experienced any issues with it personally, so I kinda thought that Better Questing was just safely deferring the execution of commands until it's finished doing what it has to do (but maybe I was just lucky!). To be honest, when I noticed that the 'default load' command accepted a filename as a parameter, I immediately thought we'd be able to make use of it to add/remove entire quest lines, or even change 'pack modes', on the fly, using better questing itself & command rewards to activate such things!

[Funwayguy]

I do intend on making an official method but I'd need to add some sort of versioning tracking to the database files first and then add additional UI elements to track those. Trying to finish cleaning up other areas first because, as much as that is a planned feature, it's not a high priority compared to the other things on the todo list for BQ3's ongoing rewrites.

For the most part it does defer manual claims to the server thread separate from the quest processing loop but the issue in this case is on the client side where the quest GUI is open. When the server finishes replacing the database the GUI is suddenly completely orphaned from the database with no way to identifying itself to other quests or the client side GUIs, thus ending up with that error. Now this could be temporarily fixed with some extra checks in the GUI but there's a bigger issue.

If you were to run this as part of an auto-claim, then the results would be much worse because those are performed while looping over the database. The remaining quests yet to be processed would also be left orphaned from the database causing mayhem and errors down the line.

If others have somehow made this work without crashing, then all the best to them, because I would not recommend it.

The proper solution to all this, and what I planned to do, is schedule the replacement between processing loops in a controlled manner. This can all be done once upon login before any GUI opens where I can compare the versions (when I add them) then replace as necessary.

Now if I can have everything stable and working for this then maybe later down the line I could look into supporting hot swapping individual quests or dynamically modifying them based on previous quests. Until then, concurrent modifications like these aren't supported and will like just break something.

[luke-bravenboer]

Sweet, great to hear, you do you! Makes sense, auto-claim sure does complicate things more than I'd considered.

Cheers for the detailed responses (& all your hard work on this superb mod, big fan!)