jpahullo / moodle-tool_mergeusers

Merge users script for Moodle
https://moodle.org/plugins/view.php?plugin=tool_mergeusers
22 stars 51 forks source link

Potential coding error - active database transaction detected during request shutdown: #258

Closed haunted2 closed 11 months ago

haunted2 commented 11 months ago

Hello, i implemented a local plugin in which a call the mergeusers plugin like this:


require_once dirname(__FILE__, 4) . '/admin/tool/mergeusers/lib/mergeusertool.php';

...

$this->merger = new MergeUserTool();

foreach ($this->fromids as $k => $fromid) {

   $this->merger->merge($this->toid, $fromid);

  }

while I'm running a post request to this script, the request aborts (during the first iteration of the loop). Looking at the php-fpm logs, I find the following error:


[13-Nov-2023 13:55:20] WARNING: child 709176 said into stderr: "NOTICE: PHP message: Potential coding error - active database transaction detected during request shutdown:"
[13-Nov-2023 13:55:20] WARNING:  child 709176 said into stderr: "* line 261 of /lib/dml/moodle_read_slave_trait.php: call to moodle_database->start_delegated_transaction()"
[13-Nov-2023 13:55:20] WARNING: child 709176 said into stderr: "* line 246 of /admin/tool/mergeusers/lib/mergeusertool.php: call to mysqli_native_moodle_database->start_delegated_transaction()"
[13-Nov-2023 13:55:20] WARNING:  child 709176 said into stderr: "* line 192 of /admin/tool/mergeusers/lib/mergeusertool.php: call to MergeUserTool->_merge()"
[13-Nov-2023 13:55:20] WARNING: [ child 709176 said into stderr: "* line 47 of /local/arpa/classes/account_merger.php: call to MergeUserTool->merge()"
[13-Nov-2023 13:55:20] WARNING: child 709176 said into stderr: "* line 163 of /local/arpa/accounts.php: call to TrioAccountMerger->merge()"

Any idea about the cause of this problem?

By merging from the plugin administration, everything works.

Thank you.

jpahullo commented 11 months ago

Hi @haunted2 ,

Thanks for reporting.

Due to Merge Users account configuration, it can rollback any possible change in presence of any error.

So, for every user being merged, for any merge(), internally it starts and concludes a database transaction.

Maybe you have started another transaction in your local_arpa plugin before invoking the merge()? It would explain this stack trace. If so, you have to commit the transaction before invoking any merge().

Let me know.

Thanks,

Jordi

haunted2 commented 11 months ago

Thanks a lot for your response. I didn't actually initiate any transactions, at least not explicitly

I tried this code just before calling $this->merger->merge:

      $transaction = $DB->start_delegated_transaction();
      $transaction->allow_commit();

but i get the same stack.

jpahullo commented 11 months ago

Hi @haunted2

Well, two things:

  1. It seems you "reinvent the wheel", since this plugin allows you to redefine the gathering key from the plugin config, and you do not need to reinvent the whole process. You only need to override the gathering key with a class name that may exist anywhere, that implements the Gathering interface from the plugin, that will return the list of users to merge, override plugin settings with a (new ) config/config.local.php file within this plugin, and call to php admin/tool/mergeusers/cli/climerger.php. We did this way in our institution in production to load them from an Oracle database. See the README.md also. I can explain further if you need so.
    • In other words, I do the same like you, but with the expected process from this plugin, and we do not see any problem about that.
  2. It seems, somehow, the process is being halted, since the first line of the stack trace is shown only from shutdown_handler.php. Double check or debug (step by step) how you reach that point.

Thanks for feedback.

Jordi

haunted2 commented 11 months ago

Thank you. I can't use the cli merger because i need to show the merge progress tu the user. From admin interface it works normally: /admin/tool/mergeusers/index.php I have to do the same thing but the with the process triggered by the user itself.

jpahullo commented 11 months ago

Hi @haunted2 ,

Maybe you can inspire yourself starting from de cli/climerger.php and so on.

As I said, we iterate over several users most of the times, every day, and we do not see any transaction error. So, the problem should appear in your workflow. I would start investigating (var_dumping, debugging step by step with your IDE, or similar) your workflow. I breakpoint into the start transaction point should be a must, to detect who starts a transaction and at which point, if possible.

I will leave this issue open for a while, in case you need more feedback. If no news, I will close it. But, feel free to create any other issue whenever necessary.

Thanks for reporting.

Jordi

haunted2 commented 11 months ago

Hello, Thank you very much. By trying the cli merger i got the same error and more information about it. It was warning coming from a script totally unrelated which caused the request shutdown. I guess Moodle was trying to rebuild the MUC cache and because of this was looking into that script.

I can confirm that by instantiating the MergeUserTool class and passing the ids, it works without problems.

jpahullo commented 11 months ago

Thanks @haunted2 for this great feedback.

That's great!