search

moodleou / moodle-report_customsql

A Moodle report plugin that lets you easily create simple reports that can be expressed as a single SQL query
46 stars 100 forks source link

Unlock session when viewing reports #113 #115

Closed brendanheywood closed 2 years ago

brendanheywood commented 2 years ago

Closes #113

timhunt commented 2 years ago

Thanks for working on this Brendan.

(A tip for the future: Given the low-bandwidth nature of issue trackers, it is a good idea to be really clear about what your plans are (or are not). E.g. you could have added a comment "I intend to work on a fix for this and send you a pull request" to #113.)

Do you have a logical reason for where the \core\session\manager::write_close(); line was added? (Forgive me, but from the force-push, one could conclude that you shoved it in randomly until the CI passed ;-))

OK, I guess generally, the earlier the better, but it needs to be after the log write. You could have included that in the comment if my guess is true, but I won't hold up the merge for that.

brendanheywood commented 2 years ago

So fun little side story on this one. I've done probably 100 of these micro patches to unlock the session all over the place and I've got a good feel for where they should go, this one ended up was exactly the way I originally wrote it and I was expecting it to be a 2 minute job. But crazy thing is it didn't work at all. I was testing with the same report open in two tabs with a pg_sleep(4) and one took 4 seconds and one took 8. I wracked my brain over it, tideways was completely lying to me and telling me that both took 4 seconds, even with early bootstrap profiling on. Weirder was that the http request was blocked rather than php running but locked at the session level. There was some weird other level of locking which was happening which I still haven't got to the bottom of. As soon as I retested using two different reports then it worked fine. But in the process I was moving the unlock all around the place trying to understand it and thats what I accidentally pushed.