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

Should the queryFactory class include a destructor? #1344

Closed
lat9 opened this issue Nov 23, 2016 · 19 comments
Closed

Should the queryFactory class include a destructor? #1344

lat9 opened this issue Nov 23, 2016 · 19 comments
Assignees

Comments

@lat9
Copy link
Contributor

lat9 commented Nov 23, 2016

As more and more sites upgrade to a version of Zen Cart that uses the mysqli interfaces, I'm seeing a significant number of sites that have logs similar to the following

PHP Warning: mysqli_connect(): (HY000/1203): User xxx already has more than 'max_user_connections' active connections in /file_path/includes/classes/db/mysql/query_factory.php on line 64

and I'm wondering if perhaps the mysqli interface is more picky than the mysql interface regarding closing down the database connection once a page's rendering is complete.

These logs are generated across Zen Cart versions 1.5.2 through 1.5.5b and on various hosted platforms, which led me to the common point of the missing class destructor. That could also be exacerbated by the issue that was present in the AJAX handler's missing load of /includes/application_bottom.php.

@drbyte
Copy link
Member

drbyte commented Nov 24, 2016

@zcwilt and I were talking about this, and tried a few dev-only tests. Seems okay with dev-only load.

If you'd like to try this in a production environment and provide feedback, it would be welcome:

function __destruct()
{
  $this->close();
}

(suggested location: just after function close() in QueryFactory)

@lat9
Copy link
Contributor Author

lat9 commented Nov 24, 2016

Sure, I'll update the queryFactory class on my personal commercial sites and suggest it to a couple of clients where I've seen those logs more frequently and report back in a couple of weeks.

Additional note: While I was looking through my sites' logs, I found another log "flavor" that I think might be related:

PHP Warning: Error while sending QUERY packet. PID=16586 in /file_system_root/includes/classes/db/mysql/query_factory.php on line 45

@lat9
Copy link
Contributor Author

lat9 commented Dec 1, 2016

So far, the change hasn't "hurt" anything, but I'm still seeing those 'max-connections' errors after the update.

@drbyte
Copy link
Member

drbyte commented Dec 1, 2016

How many "max connections" is the server's MySQL configured for?

@lat9
Copy link
Contributor Author

lat9 commented Dec 2, 2016

max_connections = 750
max_connect_errors = 100

Let me know if there are other "interesting" values.

@drbyte
Copy link
Member

drbyte commented Dec 2, 2016

My initial reaction to errors about max-connections exceeded, if it's opening and closing connections properly, is to assume that the server's MySQL config needs some tweaking to match the amount of traffic being received by the server.

The default for max_connections is often down around 100 or 150. I've seen stores bump that to 250 to handle heavy traffic needs. Given that yours is 750 it suggests maybe another issue is at hand.

Is it possible that the site has made changes that are driving "more connections" (including scaled up numbers of ajax requests), or causing connections to be kept open due to some customizations that are taking a long time to execute but not showing obvious impact on the front end to expose that there's a problem?

What are your traffic trends?
Does the problem manifest during specific times of the day?
Does it happen only when the site is being indexed by multiple search engine spiders?

This feels like it's more of a site-specific issue than a Zen Cart core code issue.

@drbyte
Copy link
Member

drbyte commented Dec 13, 2016

Touching base to see whether you discovered any related factors.

@lat9
Copy link
Contributor Author

lat9 commented Dec 15, 2016

Nothing yet; I'm waiting for the holiday shopping season to settle down before proposing the change to some of my clients (different hosts/hosting-packages with the same/similar issues).

@drbyte
Copy link
Member

drbyte commented Jan 19, 2017

If adding the destruct method was helpful, feel free to do a PR for it.

@proseLA
Copy link
Sponsor Contributor

proseLA commented Jan 20, 2017

i have started following this thread, after experiencing a similar problem. i now have a client on a vps operating for about 1 month w no problems and last night, the site experienced ~40 error logs with:

mysqli_connect(): (HY000/1040): Too many connections in /var/www/lawineco.com/public_html/includes/classes/db/mysql/query_factory.php on line 64

on this server i have:

max_connect_errors 10
max_connections 75

as there is only 1 site on this VPS, i see no need to change those numbers. also i think leaving them as is, and implementing the _destruct function may provide some good feedback. unless anyone sees a problem, i will do that now and apprise you if i continue to see these errors.

@drbyte
Copy link
Member

drbyte commented Jan 20, 2017

Yes, adding the destructor should be harmless, and if the problem persists then it helps rule it out as a cause.

Will be interesting to see what you discover with it.

If you're running a site with high traffic and have mysql max_connections at only 75 and you have more than 75 simultaneous hits you're going to run into issues. Granted, bumping that higher also likely means allocating more memory to mysql processes, depending how you've configured them. Ref: https://www.zen-cart.com/content.php?262-webserver-tuning-tips

@proseLA
Copy link
Sponsor Contributor

proseLA commented Jan 21, 2017

got it. i will leave it as is and see if the destructor helps. i think it was a bot that caused the mysql errors; i'll keep you posted.

@drbyte
Copy link
Member

drbyte commented Feb 2, 2017

Thus far I'm not seeing any noticeable differences between sites that are using a destructor as described above, vs those that aren't.

drbyte added a commit to drbyte/zencart that referenced this issue Feb 2, 2017
@lat9
Copy link
Contributor Author

lat9 commented Feb 2, 2017

Thus far I'm not seeing any noticeable differences between sites that are using a destructor as described above, vs those that aren't.

Me neither.

@proseLA
Copy link
Sponsor Contributor

proseLA commented Feb 2, 2017

i would concur as well.... i had a number of errors reported last night (first time) with the destructor in there...

@torvista
Copy link
Member

torvista commented Apr 4, 2024

From what I read in this thread max_user_connections of 25 is pretty low, which is what my shared hosting uses.
I was on a shared server and getting the error mentioned here regularly.

I moved to a VPS, but really it was over-specified for the low traffic and expensive.
To test, I set the max_user_connections to 25 for a while and had no problems.

I moved back to the shared hosting, got a couple of these in a week and yesterday in the space of 24 hours 9000 error logs concentrated in specific times.
Not had time to analyse the access logs yet. Hosting is pointing out excess traffic from a couple of bots but that would have had the same effect on the VPS, in my opinion.

On the face if it seems pretty clear that there is some issue with the shared hosting apart from the limit.
Anything further I can add into the error logging to get more info? The only thing extra that may be an issue is the ajax instant search, if that is getting hit...but I've set that delay to 450ms so multiple hits should not be a problem.

Obvious answer is move hosting (again), but I have to say they are very responsive otherwise and I want to be sure there is not something site-specific that this is highlighting.

@drbyte
Copy link
Member

drbyte commented Apr 4, 2024

I moved back to the shared hosting, got a couple of these in a week and yesterday in the space of 24 hours 9000 error logs concentrated in specific times. Not had time to analyse the access logs yet.

Ya, the analysis of that to determine what the bots were hitting could be helpful, as it may reveal a set of redirects that could be implemented in .htaccess to avoid firing up the whole framework including database.
Even if it's for sending to a 404 page that's static/cached instead of rendered from ZC's usual internals.

Hosting is pointing out excess traffic from a couple of bots but that would have had the same effect on the VPS, in my opinion.

Yes the VPS would have had to handle it regardless. But if its higher limits "tolerated" it, you may not have as many log files to assess :)

One optimization you could use to avert bots is to switch to Cloudflare for your domain's DNS. It automatically blocks known bots in real-time, and if you're under attack from extreme traffic you can temporarily turn on a captcha of sorts that will allow your server to catch up to handle legit traffic.

From what I read in this thread max_user_connections of 25 is pretty low, which is what my shared hosting uses.

While the queryFactory destructor should be adequate, you "could" add the following to /includes/application_bottom.php to specifically direct the db connection to be closed instead of relying on it to happen implicitly:

session_write_close();
+if (isset($db)) {
+    $db->close();
+}

@torvista
Copy link
Member

torvista commented Apr 5, 2024

Thanks for the detailed answer. All has been quiet until just now, so I added that last bit of code, but get
PHP Warning: Undefined property: queryFactory::$link in includes/classes/db/mysql/query_factory.php on line 637.

@drbyte
Copy link
Member

drbyte commented Apr 5, 2024

Hmm, that indicates that close() has already been called (because it calls unset($this->link).
So .. skip that code suggestion.

Back to auditing the logs and collaborating with the hosting company on what they'll do to handle load more efficiently, including blocking bot traffic. (It's in their best interest to intelligently block bot traffic when it's able to be detected.)

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

No branches or pull requests

4 participants