Moodle
  1. Moodle
  2. MDL-30140

Create token for web service admin screen out of memory error

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.1.2, 2.2, 2.3
    • Fix Version/s: 2.1.4, 2.2.1
    • Component/s: Web Services
    • Labels:
    • Environment:
      Apache
    • Database:
      MySQL, PostgreSQL
    • Testing Instructions:
      Hide

      Go to: Site administration / Plugins / Web services / Manage tokens
      (/admin/settings.php?section=webservicetokens)

      Select Add link.

      Get an out of memory error instead of expected page content.

      There are 70,000 entries in the user table on my test system.

      Page error text:

      Fatal error: Allowed memory size of 134217728 bytes exhausted (tried to allocate 74 bytes) in /lib/dml/pgsql_native_moodle_database.php on line 718 Call Stack: 0.0000 634560 1.

      {main}

      () /admin/webservice/tokens.php:0 1.2209 47867152 2. moodleform->moodleform() /admin/webservice/tokens.php:55 1.2238 47921560 3. web_service_token_form->definition() /lib/formslib.php:154 1.2262 48171944 4. pgsql_native_moodle_database->get_records_sql() /admin/webservice/forms.php:189

      Show
      Go to: Site administration / Plugins / Web services / Manage tokens (/admin/settings.php?section=webservicetokens) Select Add link. Get an out of memory error instead of expected page content. There are 70,000 entries in the user table on my test system. Page error text: Fatal error: Allowed memory size of 134217728 bytes exhausted (tried to allocate 74 bytes) in /lib/dml/pgsql_native_moodle_database.php on line 718 Call Stack: 0.0000 634560 1. {main} () /admin/webservice/tokens.php:0 1.2209 47867152 2. moodleform->moodleform() /admin/webservice/tokens.php:55 1.2238 47921560 3. web_service_token_form->definition() /lib/formslib.php:154 1.2262 48171944 4. pgsql_native_moodle_database->get_records_sql() /admin/webservice/forms.php:189
    • URL:
      /admin/settings.php?section=webservicetokens
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:
    • Rank:
      24804

      Description

      When attempting to create a web service token for a user via the web services admin section an out of memory error causes page load to fail.

      Error is from admin/webservice/forms.php

      web_service_token_form class has a select on the entire user table to populate the user 'searchableselector'.

      We have 300,000 users in the user table... so selecting the entire table is causing an issue!

      Even selecting on a system with a few thousand users means searchableselector search on the token page becomes unusable.

      Is there any data (capability check?) that could be used to cut down on the number returned or an alternate means of selecting the user that doesn't involve loading all at once (like on admin/user/user_bulk.php)?

        Issue Links

          Activity

          Hide
          Jason Platts added a comment -

          I see 'development is in progress' for this issue.

          Any ideas when it will be released?

          If it's not any time soon I'll need to do a hack to get this working on our system.

          Thanks

          Show
          Jason Platts added a comment - I see 'development is in progress' for this issue. Any ideas when it will be released? If it's not any time soon I'll need to do a hack to get this working on our system. Thanks
          Hide
          Jérôme Mouneyrac added a comment -

          Hi Jason, I was planning to release it next week. I did a nice new "user search" form element in Ajax but it still needs more works (small refactoring to have a generic version, testing, and specially no-javascript work around).

          What hack are you suggesting by the way? Replacing/addding a userid/username field? I think I will most likely have to add a field like that for the no-javascript version. I'll try to push the no-javascript version before the nice Ajaxy solution, so it is fixed very soon.

          Show
          Jérôme Mouneyrac added a comment - Hi Jason, I was planning to release it next week. I did a nice new "user search" form element in Ajax but it still needs more works (small refactoring to have a generic version, testing, and specially no-javascript work around). What hack are you suggesting by the way? Replacing/addding a userid/username field? I think I will most likely have to add a field like that for the no-javascript version. I'll try to push the no-javascript version before the nice Ajaxy solution, so it is fixed very soon.
          Hide
          Jason Platts added a comment -

          Thanks Jerome, that sounds great - I appreciate all your efforts on this and the other web service improvements, it's a fantastic feature.

          I was just going to have a local copy of the page that took a userid as a parameter to create the token - but having the core version working is going to be so much better.

          Show
          Jason Platts added a comment - Thanks Jerome, that sounds great - I appreciate all your efforts on this and the other web service improvements, it's a fantastic feature. I was just going to have a local copy of the page that took a userid as a parameter to create the token - but having the core version working is going to be so much better.
          Hide
          Jérôme Mouneyrac added a comment -

          Back to this issue

          Show
          Jérôme Mouneyrac added a comment - Back to this issue
          Hide
          Jérôme Mouneyrac added a comment -

          Hi Rosie,
          can you peer review?

          The change transforms the user select (Admin > Plugins > Web services > Manage tokens > Add) into a user text when there is more than 500 users in the all site. There is a generator in /admin/generator.php to generate lots of user in your moodle site. Can you test on 2.0 if possible? I tested with 2.2. Cheers.

          Show
          Jérôme Mouneyrac added a comment - Hi Rosie, can you peer review? The change transforms the user select (Admin > Plugins > Web services > Manage tokens > Add) into a user text when there is more than 500 users in the all site. There is a generator in /admin/generator.php to generate lots of user in your moodle site. Can you test on 2.0 if possible? I tested with 2.2. Cheers.
          Hide
          Rossiani Wijaya added a comment -

          Hi Jerome,

          Some feedback 'token' from me

          1. The second if statement need to be on new line

          if (count($users) > 1) {
             $errors['user'] = get_string('usernameoridoccurenceerror', 'webservice');
          } if (count($users) == 0) {
             $errors['user'] = get_string('usernameoridnousererror', 'webservice');
          }
          

          2. The validation works fine for invalid username. However it didn't check for userid validation. Inserting invalid id will bring user to tokens.php with no token created.

          3. Just wondering based on the code above. Since username should be unique for each user, is it necessary to check for >1 and == 0? Could it just do the following:

           if (count($users) != 1) {
             $errors['user'] = 'invalid user';
           }
          
          Show
          Rossiani Wijaya added a comment - Hi Jerome, Some feedback 'token' from me 1. The second if statement need to be on new line if (count($users) > 1) { $errors['user'] = get_string('usernameoridoccurenceerror', 'webservice'); } if (count($users) == 0) { $errors['user'] = get_string('usernameoridnousererror', 'webservice'); } 2. The validation works fine for invalid username. However it didn't check for userid validation. Inserting invalid id will bring user to tokens.php with no token created. 3. Just wondering based on the code above. Since username should be unique for each user, is it necessary to check for >1 and == 0? Could it just do the following: if (count($users) != 1) { $errors['user'] = 'invalid user'; }
          Hide
          Rossiani Wijaya added a comment -

          Hi Jerome,

          Other than the above comments, the patch works fine in all 2.x versions.

          Show
          Rossiani Wijaya added a comment - Hi Jerome, Other than the above comments, the patch works fine in all 2.x versions.
          Hide
          Jérôme Mouneyrac added a comment -

          Thank you Rosie. I'll fix 1. and 2. I think it's good to have two different messages to help the admin.

          Show
          Jérôme Mouneyrac added a comment - Thank you Rosie. I'll fix 1. and 2. I think it's good to have two different messages to help the admin.
          Hide
          Sam Hemelryk added a comment -

          Hi Jerome,

          I'm reopening this at the moment so that a couple of minor things can be looked at.

          1. When searching by username you need to clean username using PARAM_USERNAME before searching the DB. Saves you a query for invalid usernames and is more secure.
          2. The new strings need reviewing. You refer to user ID in 3 ways (ignoring capitalisation): user id, userid, and id. I would vote for user id.
          3. Bad punctuation in usernameoridoccurenceerror and probably needs review anyway. Perhaps 'More than one user matches the username you entered. You will need to use the users id.'... On an interesting note username is unique to a site, so this can technically only happen if a username exists in two hosts joined by mnet.

          I'd perhaps get Helen to look over the strings and review them as my English is pretty average.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Jerome, I'm reopening this at the moment so that a couple of minor things can be looked at. When searching by username you need to clean username using PARAM_USERNAME before searching the DB. Saves you a query for invalid usernames and is more secure. The new strings need reviewing. You refer to user ID in 3 ways (ignoring capitalisation): user id, userid, and id. I would vote for user id. Bad punctuation in usernameoridoccurenceerror and probably needs review anyway. Perhaps 'More than one user matches the username you entered. You will need to use the users id.'... On an interesting note username is unique to a site, so this can technically only happen if a username exists in two hosts joined by mnet. I'd perhaps get Helen to look over the strings and review them as my English is pretty average. Cheers Sam
          Hide
          Jérôme Mouneyrac added a comment -

          Thanks Sam, fixing it

          Show
          Jérôme Mouneyrac added a comment - Thanks Sam, fixing it
          Hide
          Tim Hunt added a comment -

          Why are we implementing a new user search system. Surely this UI should be using the user selector UI that is used in places like assign system roles, or on .../admin/webservice/service_users.php?id=2

          That automatically gets the searching right, and avoids having to directly refer to userid.

          Show
          Tim Hunt added a comment - Why are we implementing a new user search system. Surely this UI should be using the user selector UI that is used in places like assign system roles, or on .../admin/webservice/service_users.php?id=2 That automatically gets the searching right, and avoids having to directly refer to userid.
          Hide
          Sam Hemelryk added a comment -

          Hi Tim, thanks for looking at this.

          When I first looked at it that was the first thing the sprung to mind.
          However I'm not overly familiar with the user selector so I grepped for areas where it is being used but after looking at them I wasn't sure that it was the right way to go right now.
          The mform Jerome is working on here is used to select a single user, and works within an admin form so there are no capability checks or other additional checks that it requires.
          It does mean his use is simple and it wouldn't be a hard task to create a user selector for the webservice token form. But it does mean he would need to create that user selector and convert the script away from using an mform.
          Certainly its work worth doing, but I get the impression that Jerome created MDL-30537 for that, the changes he has here fix the form for sites with large numbers of users in a temporary way.
          I suppose the question is do we get this in now and then use MDL-30537 to improve our integration properly, either converting away from the mform, or integrating the user selector with an mform element. Or we aim to get it all sorted now in this issue and not have the intermediary issue.

          Personally I am conflicted on this issue. We spawn a lot of issues that never get looked at again, so I am hesitant on trusting MDL-30537 will do done any time soon.
          However this solution is here, and it works, and I don't know what Jerome's priorities are like, perhaps if we require this to be fixed with a user selector it won't get done because it's not priority (this was a quick fix).

          Tim, Jerome, what are your thoughts?
          Now or later?

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Tim, thanks for looking at this. When I first looked at it that was the first thing the sprung to mind. However I'm not overly familiar with the user selector so I grepped for areas where it is being used but after looking at them I wasn't sure that it was the right way to go right now. The mform Jerome is working on here is used to select a single user, and works within an admin form so there are no capability checks or other additional checks that it requires. It does mean his use is simple and it wouldn't be a hard task to create a user selector for the webservice token form. But it does mean he would need to create that user selector and convert the script away from using an mform. Certainly its work worth doing, but I get the impression that Jerome created MDL-30537 for that, the changes he has here fix the form for sites with large numbers of users in a temporary way. I suppose the question is do we get this in now and then use MDL-30537 to improve our integration properly, either converting away from the mform, or integrating the user selector with an mform element. Or we aim to get it all sorted now in this issue and not have the intermediary issue. Personally I am conflicted on this issue. We spawn a lot of issues that never get looked at again, so I am hesitant on trusting MDL-30537 will do done any time soon. However this solution is here, and it works, and I don't know what Jerome's priorities are like, perhaps if we require this to be fixed with a user selector it won't get done because it's not priority (this was a quick fix). Tim, Jerome, what are your thoughts? Now or later? Cheers Sam
          Hide
          Tim Hunt added a comment -

          I am happy if this gets fixed quickly, and MDL-30537 is done later.

          We really need a version of userselector wrapped up in a form field type.

          Show
          Tim Hunt added a comment - I am happy if this gets fixed quickly, and MDL-30537 is done later. We really need a version of userselector wrapped up in a form field type.
          Hide
          Sam Hemelryk added a comment -

          Thanks for the quick response Tim, will wait for Jerome to come online and get his thoughts.

          Show
          Sam Hemelryk added a comment - Thanks for the quick response Tim, will wait for Jerome to come online and get his thoughts.
          Hide
          Jérôme Mouneyrac added a comment -

          Hi all,
          when I started to work on this issue I thought about what would be the best way to select a user. My preference is something like the Facebook selector. It's a simple text field with autocompletion. It's small, easy to use, instantaneous and efficient.

          During my coding phase I remembered our current user's selector. I tested it but it didn't work with a huge number of users. I generated 500000 users with our admin generator and tried to search for 'alice' but it didn't return anything. I also noticed it was a bit slow compare to what I wanted to obtain (no more than 2 seconds and only for some cases).

          At the end I kept going to work a day on the alternative I was coding because I wanted to see if I could obtain the Facebook-like alternative quickly. I guess the alpha result I got in MDL-30537 is quite nice (very fast in most cases, search on email/username/firstname/lastname/both, display user pictures, fast user selection). However I had to focus on other more important tasks in the meanwhile.

          I'm still not happy yet with the fact to give up on left %, I want to try to copy the user table into a file(s) to see how big the file would be for few millions of user and how fast it can be browsed.

          So in conclusion MDL-30537 has been in stand-by and is not for soon. Specially that the design hasn't been approved by anyone. It's just my vision of the user selector.

          For this issue MDL-30140, I did fix the last Sam's comment, and I was about to test it this morning. But I can do the switch for our current selector. I agree it's more friendly than username/user id. It is just going to take a bit more time, as I said it's buggy with huge number of users.

          In conclusion, I'll push my changes. You decide or not to integrate it. I, or someone else, can work on the switch to our user selector.

          Show
          Jérôme Mouneyrac added a comment - Hi all, when I started to work on this issue I thought about what would be the best way to select a user. My preference is something like the Facebook selector. It's a simple text field with autocompletion. It's small, easy to use, instantaneous and efficient. During my coding phase I remembered our current user's selector. I tested it but it didn't work with a huge number of users. I generated 500000 users with our admin generator and tried to search for 'alice' but it didn't return anything. I also noticed it was a bit slow compare to what I wanted to obtain (no more than 2 seconds and only for some cases). At the end I kept going to work a day on the alternative I was coding because I wanted to see if I could obtain the Facebook-like alternative quickly. I guess the alpha result I got in MDL-30537 is quite nice (very fast in most cases, search on email/username/firstname/lastname/both, display user pictures, fast user selection). However I had to focus on other more important tasks in the meanwhile. I'm still not happy yet with the fact to give up on left %, I want to try to copy the user table into a file(s) to see how big the file would be for few millions of user and how fast it can be browsed. So in conclusion MDL-30537 has been in stand-by and is not for soon. Specially that the design hasn't been approved by anyone. It's just my vision of the user selector. For this issue MDL-30140 , I did fix the last Sam's comment, and I was about to test it this morning. But I can do the switch for our current selector. I agree it's more friendly than username/user id. It is just going to take a bit more time, as I said it's buggy with huge number of users. In conclusion, I'll push my changes. You decide or not to integrate it. I, or someone else, can work on the switch to our user selector.
          Hide
          Jérôme Mouneyrac added a comment -

          I had another look the user selector is not a form element as Sam mentioned, I forgot this detail I think MDL-30537 is definitively worth as it is a form element. I'm pushing my temporary changes and we'll decide later if we prefer to finish MDL-30537 or refactor the entire page 'Add token' to support the user selector.

          Show
          Jérôme Mouneyrac added a comment - I had another look the user selector is not a form element as Sam mentioned, I forgot this detail I think MDL-30537 is definitively worth as it is a form element. I'm pushing my temporary changes and we'll decide later if we prefer to finish MDL-30537 or refactor the entire page 'Add token' to support the user selector.
          Hide
          Tim Hunt added a comment -

          OK, so this fix should be integrated.

          (Except that, in Moodle, we have a policy of NEVER displaying, or searching on, username.)

          Now, on the subject of the user selector, it seems pretty silly to say "I think this standard component that we use in lots of places in Moodle sucks (in some situations) so I am going to re-implement my own completely separate widget that fixes the one problem I care about."

          • It means the UI in not consistent for users
          • It means we have two sets of code to maintain in the future
          • It does not fix the performance issues in all the places the user selector is used.
          • You are currently obsessing about performance, but that in only one thing. Your new code will probably have other non-performance problems that the existing user selector has already solved.

          Anyway, enough ranting. Suffice it to say, that if there is a faster way of doing things, then the main user-selector should be changed to work that way. (Mind you, the users selector seems fine on the OU's servers so far. I wonder how many users we have in our DB. When you say the user-selector is too slow with half a million users, what hardware are you running on? Large Moodle sites tend to run on serious servers, not a desktop PC.)

          Show
          Tim Hunt added a comment - OK, so this fix should be integrated. (Except that, in Moodle, we have a policy of NEVER displaying, or searching on, username.) Now, on the subject of the user selector, it seems pretty silly to say "I think this standard component that we use in lots of places in Moodle sucks (in some situations) so I am going to re-implement my own completely separate widget that fixes the one problem I care about." It means the UI in not consistent for users It means we have two sets of code to maintain in the future It does not fix the performance issues in all the places the user selector is used. You are currently obsessing about performance, but that in only one thing. Your new code will probably have other non-performance problems that the existing user selector has already solved. Anyway, enough ranting. Suffice it to say, that if there is a faster way of doing things, then the main user-selector should be changed to work that way. (Mind you, the users selector seems fine on the OU's servers so far. I wonder how many users we have in our DB. When you say the user-selector is too slow with half a million users, what hardware are you running on? Large Moodle sites tend to run on serious servers, not a desktop PC.)
          Hide
          Jérôme Mouneyrac added a comment -

          This new username search field is only displayed when the site has a lot of users, and on this very rarely used page. But if we have the policy to never search on username, we do not integrate this fix.

          So if everybody is ok to wait a bit more, I'll refactor this token page. Sam can you give me back the issue?
          Admins can still cherry-pick the new username search field, if they want a temporary workaround.

          Out of topic about the form element I developed:
          Tim, I agree with all the points you mentioned (consistency issue, two set of code, does not fix our current selector performance, my mac is not setup for big site). But you forgot to mention that it is a form element, not our current selector. Moreover I developed it as something I believe was a nice improvement. If nobody like it, we close the MDL-30537 issue, easy

          Show
          Jérôme Mouneyrac added a comment - This new username search field is only displayed when the site has a lot of users, and on this very rarely used page. But if we have the policy to never search on username, we do not integrate this fix. So if everybody is ok to wait a bit more, I'll refactor this token page. Sam can you give me back the issue? Admins can still cherry-pick the new username search field, if they want a temporary workaround. Out of topic about the form element I developed: Tim, I agree with all the points you mentioned (consistency issue, two set of code, does not fix our current selector performance, my mac is not setup for big site). But you forgot to mention that it is a form element, not our current selector. Moreover I developed it as something I believe was a nice improvement. If nobody like it, we close the MDL-30537 issue, easy
          Hide
          Tim Hunt added a comment -

          Well, it would be good to have a version of the userselector UI optimised for selecting a single user, rather than multiple - but it should use the same back-end code as the select-multi selector. Also, the whole user-selector thing should be re-done to separate the back-end DB code from the front-end, that should be generated with a renderer (with two methods, one for single, one for multi). So, MDL-30537 should be kept open, but we need to do a proper job there.

          Show
          Tim Hunt added a comment - Well, it would be good to have a version of the userselector UI optimised for selecting a single user, rather than multiple - but it should use the same back-end code as the select-multi selector. Also, the whole user-selector thing should be re-done to separate the back-end DB code from the front-end, that should be generated with a renderer (with two methods, one for single, one for multi). So, MDL-30537 should be kept open, but we need to do a proper job there.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Sam Hemelryk added a comment -

          Giving this back to Jerome as requested. (will also remove from this sprint)

          Show
          Sam Hemelryk added a comment - Giving this back to Jerome as requested. (will also remove from this sprint)
          Hide
          Jérôme Mouneyrac added a comment -

          No worries. Note that with my current priorities and the January docs only month, I'll not be back on this issue before February. You can cherry-pick my temporary fix for the moment, I retested the last version it works. Don't hesitate to ping me in February if you don't see any activities on it.

          Thanks for your comprehension. Cheers.

          Show
          Jérôme Mouneyrac added a comment - No worries. Note that with my current priorities and the January docs only month, I'll not be back on this issue before February. You can cherry-pick my temporary fix for the moment, I retested the last version it works. Don't hesitate to ping me in February if you don't see any activities on it. Thanks for your comprehension. Cheers.
          Hide
          Tim Hunt added a comment -

          Grrr! This page currently does not work at all in sites with many users. We have a fix that seems to work, even if it is not perfect. If Jerome really cannot fix this until February, then why not commit the current, imperfect, fix, and open a new issue for Feb?

          Show
          Tim Hunt added a comment - Grrr! This page currently does not work at all in sites with many users. We have a fix that seems to work, even if it is not perfect. If Jerome really cannot fix this until February, then why not commit the current, imperfect, fix, and open a new issue for Feb?
          Hide
          Jérôme Mouneyrac added a comment -

          Tim, I'm pretty sure Sam will integrate it if you need it quickly

          Show
          Jérôme Mouneyrac added a comment - Tim, I'm pretty sure Sam will integrate it if you need it quickly
          Hide
          Sam Hemelryk added a comment -

          Thanks fella's eventually this has made it

          Show
          Sam Hemelryk added a comment - Thanks fella's eventually this has made it
          Hide
          Rajesh Taneja added a comment -

          Works fine
          Page loads without user select box, but with text box.
          Thanks for fixing this Jerome.

          Show
          Rajesh Taneja added a comment - Works fine Page loads without user select box, but with text box. Thanks for fixing this Jerome.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Whoever decided one week was worth 14 days had really one bad idea. Anyway, the nightmare is over, so thanks for your, once again, amazing contributions. Many, many thanks!

          Now... disconnect, relax and enjoy the next days, yay!

          Closing...ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Whoever decided one week was worth 14 days had really one bad idea. Anyway, the nightmare is over, so thanks for your, once again, amazing contributions. Many, many thanks! Now... disconnect, relax and enjoy the next days, yay! Closing...ciao

            People

            • Votes:
              3 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: