strongloop / strong-supervisor

Application supervisor that automatically adds cluster control and performance monitoring with StrongOps
Other
66 stars 20 forks source link

JSON.stringify crash #203

Closed fcarreiro closed 8 years ago

fcarreiro commented 8 years ago

I would like to point you to https://github.com/strongloop/strong-pm/issues/307 since it may actually concern this subproject.

rmg commented 8 years ago

@sam-github this has been around for a while, but I'm pretty sure #199 would resolve it since it completely removes the feature with the bug. Thoughts?

fcarreiro commented 8 years ago

I'm trying to isolate this "bug", since it only happens to my code base when I move from mysql with explicit connections to connection pooling (which uses EventEmmiter in a more fundamental way). It is probably not just a Strongloop bug. However, it is proving extremely difficult to isolate. I'll let you know if I can do it.

rmg commented 8 years ago

@fcarreiro that's actually a pretty useful clue. I'm not familiar with the mysql module code, but you seem like you might be. Maybe you can corroborate this theory:

If Connection.prototype.query receives an object as it's sql argument instead of a string, then that might throw off the probe which appears to be expecting a string. If an object is passed and intercepted there, it would propagate to the top-calls collection where it would be stored until forwarding to strong-supervisor where it would meet up with the stackrace you provided in https://github.com/strongloop/strong-pm/issues/307#issuecomment-242933390.

fcarreiro commented 8 years ago

I have to admit that, so far, I've only used Strongloop as a process manager. I was not aware that it was intercepting calls to the mysql code (as I'm not using it for profiling, etc.) Regarding mysql, I'm no expert at all :) but I'm trying to sort this out.

The call I'm using is actually Pool.prototype.query, for the record. I have just tried passing objects as the sql and it seems to respond ok. That is, error, but no crash in Strongloop :/

rmg commented 8 years ago

@fcarreiro sorry, I'm not sure what I was thinking before! It couldn't be related to MySQL because those metrics don't even propagate through strong-supervisor. :blush:

It is the http probe, specifically for your request handlers, that would be the origin of the mishandled data.

Do you (or your framework/middleware) do anything with your request objects to make the url property an object instead of a string? It should be pretty straight forward to inject some debugging output into your app to find out :-)

fcarreiro commented 8 years ago

I'll check! I use express, and have many middlewares. However, it still puzzles me that I only can reproduce this error if I use mysql connection pooling! Does Strongloop do something with the errors propagated through the middlewares? Because there I am certainly using objects there (well, JSON).

fcarreiro commented 8 years ago

@rmg Hmm, the urls seem ok, in general. One question, does Strongloop maybe save the whole req as a context or something like that? It may be nonsense but now I'm wondering if something like this could be happening: Using passportjs I deserialize some user into req.user. What I assign to user is actually the raw result from a mysql query (AFAIK, that's what people do). I think this result has some mysql metadata. It could be the case that that object contains some reference to, say, the connection. The connection is clearly (sub)process dependent and will not stringify nicely in the top calls. Moreover, the connections get invalidated more often/carry more data when using connection pooling.

It's a long shot, but maybe rings some bell.

Update: I've just returned {} in the deserialization and it still happens. It could still be some other db call in my code (related or not to req) but no success for the moment.

Update: I've checked the results of every db call (that my app does, not the imported middleware) with inspect and they don't seem to have any weird db info.

fcarreiro commented 8 years ago

@rmg I see that you merged #204, when it makes it to a release let me know and I'll report on that ;)