Configuration is not always saved

[whimboo]

I get a ping from @FlorinMezei this morning regarding his admin privileges. Those were gone again since we restarted the bot yesterday. I have had a look into the code and have seen that the configuration for at least admins is not saved when the list is changing. So if the bot is restarted by the system daemon due to updates the configuration is gone.

So for all the data stored in the config file and we are able to change inside the bot, a saveData() call has to happen.

[xabolcs]

I'd like to work on this issue.

[whimboo]

Thanks! I have assigned you now so everyone knows. :)

[xabolcs]

Testing with the patch applied below, it seems like it saves the data all the time.

diff --git a/bot.js b/bot.js
index 13f6e2d..6d51871 100644
--- a/bot.js
+++ b/bot.js
@@ -440,6 +440,7 @@ client.addListener('pm', function(from, message) { // private messages to bot
     }

     saveData("testDay", JSON.stringify(testDay));
+    console.log('End of whois function');
   });
 });

What if something bad happened and an error was logged into the console?

It would be nice if saveData could provide a feedback somehow, e.g. rollback the unsaved change or something.

[whimboo]

Hm, I wouldn't send a message each time time we safe but it sounds good to inform the admin, who made the changes, that the save action was not successful. Best is always to keep the changes in temporary variables and update the global ones when the safe action is done.

[xabolcs]

[...] it sounds good to inform the admin, who made the changes, that the save action was not successful

For example with callbacks? Or with promises instead? :)

[whimboo]

Depends on. What I know from the work of Barbara we hit an issue when the global error handler was called in case of exceptions. Not sure if that applies to every one. In that case it might indeed become harder, but we should try to check how node.js reacts on such a kind of failure.

[xabolcs]

Sorry?

I thought including a simple callback near #L529.

[whimboo]

Sorry, forget my last comment then. Now I see that we have an error callback. When it works we are fine here. So instead of writing to the console, shall we better send a message with the failure to the user who triggered this change? I don't think anyone will check the console.

[xabolcs]

I'll give a try to extend saveData with a callback.

Did you know that it is unsafe to use fs.write multiple times on the same file without waiting for the callback? Quote from fs.write documentation:

Note that it is unsafe to use fs.write multiple times on the same file without waiting for the callback. For this scenario, fs.createWriteStream is strongly recommended.

Should I introduce a saveDataSemaphore instead start using streams?

[whimboo]

I think that this is a case which normally would never happen. The write operation should finish within hundreds of milliseconds. That is not a time a user would send multiple requests to the bot. What happens right now when we set a new value that quickly? Would it call the error callback? We could let the user know that way about the write failure.

[xabolcs]

That is not a time a user would send multiple requests to the bot.

It's good enough if multiple user send a request to the bot at the same time

What happens right now when we set a new value that quickly? Would it call the error callback? We could let the user know that way about the write failure.

I didn't started the work yet. But yes, an error callback which sends the error to the user would be enough for now.

[whimboo]

I think so too. Lets do the trivial way. The bot is actually not a high-end product, and as long as we report such a failure back to the user he will know that the action needs to be repeated.

[xabolcs]

Example talk:

[02:39:18]  xabolcs :admin remove korte
[02:39:18]  xabolcs_tdb Test Day admins are xabolcs
[02:39:18]  xabolcs_tdb Saving testDay data was unsuccessful. Please repeat the command!
[02:39:18]  xabolcs_tdb The error was: Error: EACCES, open '/root/data/testDay.json'

Looks good?

[whimboo]

Yes, that looks wonderful!

[xabolcs]

Thanks! Let's review the referencing PR! :)

[whimboo]

PR #81 has been merged.