Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sails request hangs on sails.getDataStore().transaction(fn) , and fn seems not executed when sails-mysql upgraded to 1.0.0-16 #4271

Closed
guretno opened this issue Dec 28, 2017 · 20 comments

Comments

@guretno
Copy link

guretno commented Dec 28, 2017

Sails version: 1.0.0-37
Node version: 6.10.0 ,8.9.3
NPM version: 3.10.10, 5.5.1
DB adapter name: sails-mysql
DB adapter version: 1.0.0-16
Operating system: Linux (Amazon Linux AMI), MacOS 10.13.1


Hi, 5 days ago (on 23/12/17) suddenly I notice that my datastore transaction is not working and from the logs of my server, I saw that when there is a request, the processing hangs on sails.getDataStore().transaction(fn) and it seems the fn was not getting executed. I'm using sails v1.0 and I've tried in both node 6.10.0 LTS and 8.9.3 LTS and the issue persists.

Then later I found out that the sails-mysql is updated to 1.0.0-16 , 7 days ago (on 21/12/7) from sails-mysql git repo and since sails js framework setting on sails-mysql : ^1.0.0-14, hence it updated to 1.0.0-16 when i reinstall (npm install) and rebuild the package. The issue was not happening on sails-mysql 1.0.0-15.

How I'm using the data store transaction is as follows, (which I guess from the previous example from sails documentation),

 sails.getDatastore().transaction(function (db, proceed) {
      async.waterfall([
        function(done) {
           // do something
           return done(null);
        }
      ], function(err) {
        if (err) {
           return proceed(err); 
        }
        return proceed(); 
        
      });
    
 }).exec(function(err){
   return res.ok()
});

Which I notice that the example on the implementation has been changed here https://next.sailsjs.com/documentation/reference/waterline-orm/datastores/transaction.

I tried to use 'await' but it always throws syntax error : Unexpected identifier. My current implementation is fine with sails-mysql version 1.0.0-15, so does the issue is because the way the transaction function need to be defined with async (if want to use v 1.0.0-16) like in the current example on sails documentation or my current implementation is fine but there are some steps i have missed here?

Thanks a lot in advance for any help/assistance provided.

@sailsbot
Copy link

@guretno Thanks for posting, we'll take a look as soon as possible.


For help with questions about Sails, click here. If you’re interested in hiring @sailsbot and her minions in Austin, click here.

@akraminakib
Copy link

akraminakib commented Jan 7, 2018

I'm also getting hanging request issues, but with Model.getDatastore().sendNativeQuery(). I'm using sails-mysql 1.0.0-16 as well.

@moty66
Copy link

moty66 commented Jan 8, 2018

I confirm this issue

sails                 1.0.0-45
sails-disk            1.0.0-12
sails-hook-grunt         1.0.8
sails-hook-orm        2.0.0-22
sails-hook-sockets       1.5.1
sails-mongo           1.0.0-11
sails-mysql           1.0.0-16

@mokxter
Copy link

mokxter commented Jan 10, 2018

I can also confirm this issue. I'm using sails 1.0.0.0-45 and sails-mysql 1.0.0-16.

@TomislavStrugacevac
Copy link

TomislavStrugacevac commented Jan 11, 2018

Confirmed as well,:

    "sails": "^1.0.0-45",
    "sails-hook-orm": "^2.0.0-22",
    "sails-hook-sockets": "^1.5.1",
    "sails-mysql": "^1.0.0-16"

@moty66
Copy link

moty66 commented Jan 11, 2018

This is a blocking issue at least for me, downgrading sails-mysql to 1.0.0-15 does not fix this problem.

@itomas
Copy link

itomas commented Jan 11, 2018

@moty66 try to remove node_modules, clear cache and etc.. 15v should fix.

@smihaljenovic
Copy link

Yes, for us, downgrading sails-mysql to 1.0.0-15 helped to solve this issue.

@fenichelar
Copy link

I also have the same issue with Model.getDatastore().sendNativeQuery(). Downgrading sails-mysql to 1.0.0-15 resolved the issue.

@mikermcneil
Copy link
Member

@guretno Just to clarify, in this example, it looks like you would expect a hung request if any error occurred, since the error isn't being handled in the final .exec() callback. Would you mind confirming that you still see the same behavior when you add if (err) { return res.serverError(err); } above the call to res.ok()?

@fenichelar
Copy link

fenichelar commented Jan 25, 2018

@mikermcneil Even if err is not checked, I would expect res.ok() to still be executed thus it wouldn't hang?

At least in my case (no transaction), the exec callback is never executed. Confirmed with console.log('exec'); as the first line of the callback.;

@guretno
Copy link
Author

guretno commented Jan 25, 2018

@mikermcneil Thanks for looking at this issue. Yep still the same, it seems the .exec() callback is not getting called.

Does it have something to do with the upgrade of the driver machinepack-mysql in sails-mysql? I saw that the datastore and hence the transaction as well as send native query function are handled by that and it's being upgraded to v3.0.0 in sails-mysql 1.0.0.16 to solve a memory leak issue (#4264).

Thanks a lot!

@mikermcneil
Copy link
Member

@mikermcneil
Copy link
Member

Does it have something to do with the upgrade of the driver machinepack-mysql in sails-mysql?

@guretno yeah good call, quite possibly-- could also be the runner itself

@mikermcneil
Copy link
Member

@guretno @sgress454 @fenichelar @itomas @smihaljenovic @moty66 @akraminakib @mokxter @TomislavStrugacevac Hey everyone, thanks for the help. I just published a release of sails-hook-orm (2.0.0-23) with a fix. To pick that up, run npm install sails-hook-orm@^2.0.0-23 in your project (or if you prefer to pin your semver ranges, do npm install sails-hook-orm@2.0.0-23 --save-exact).

Here's what happened: First of all, this bug was particularly pernicious because machine wasn't properly informing us that something was amiss (it wasn't throwing this error like it should have been). So I went ahead and corrected that and published as machine@15.0.0-22.

Now for the actual bug: For compatibility, sails-hook-orm calls the driver directly rather than using the adapter within its implementation of the RDI methods-- that is, the datastore methods available in userland (sendNativeQuery(), transaction(), etc). But when I bumped the runner version in the mp-mysql driver, I didn't update the usage in sh-orm. That's fixed now.

@guretno @sgress454 @fenichelar @itomas @smihaljenovic @moty66 @akraminakib @mokxter @TomislavStrugacevac I verified that this patch fixes the repro here, but when y'all have a moment, would you try that out and make sure everything looks good in your apps?

@ultamatt
Copy link

ultamatt commented Jan 30, 2018

Yo @mikermcneil In a strange twist, I find myself the first (I think) to google this issue and find your patch. I'm on sails 1.0.0-37, and sails-mysql 1.0.0-15 and grabbed sails-hook-orm 2.0.0-23 as you put above. Still getting no return from native query. (also tried replacing .exec( with .switch( and appropriate object for fun and no luck there either)

How can I help you solve this problem? (which is also my problem now) Need some more package.json info?

@guretno
Copy link
Author

guretno commented Jan 31, 2018

@mikermcneil , Yes it is fine now, the transaction callback is called. perfect, Thanks! I changed the sails-hook-orm to 2.0.0-23 and sails-mysql to 1.0.0-17. I see the machine also has been upgraded to machine@15.0.0-22.

    "sails": "^1.0.0-37",
    "sails-disk": "~0.10.0",
    "sails-hook-grunt": "^2.0.0",
    "sails-hook-orm": "2.0.0-23",
    "sails-hook-sockets": "^1.4.0",
    "sails-mysql": "1.0.0-17",

@ultamatt , I guess you can try to remove the sails node_modules folder and try to npm install again, as some dependencies e.g machines in sails-mysql@1.0.0-17 has been updated as well.

@sgress454
Copy link
Member

@mikermcneil confirmed fix here as well. @ultamatt I second the rm -rf node_modules && npm install!

@ultamatt
Copy link

I did nuked my node_modules and npm install'd as y'all suggested and now i'm right as rain. Thank you everyone!

@fenichelar
Copy link

@mikermcneil :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests