node-mysql2 icon indicating copy to clipboard operation
node-mysql2 copied to clipboard

fix: resolve LRU conflicts and premature engine breaking change

Open wellwelwel opened this issue 1 year ago β€’ 2 comments

Fixes #1885, Fixes #1953, Fixes #1965, Fixes #1970, Fixes #2001, Fixes #2537, Fixes #2619, Fixes #2752.


A quick explanation

[!IMPORTANT]

This PR replaces the lru-cache.

The motivation πŸ§‘πŸ»β€πŸ”¬

The two most used projects (lru-cache and quick-lru) that offer the most complete solutions usually dropping Node.js versions (and it's fine). But this situation requires maintainers to also release a major version or continue to use a discontinued/old version.

Based on 7 of 8 linked issues, it started to cause a cascading reaction when each different package made a different decision, causing conflicts when installing the same package in different major versions of the same root project (it's not only a MySQL2 problem).

The solution πŸ’‘

Then I noticed that there was no need for literally ANY pollyfill (even after compilation) to run a LRU cache with even a little more performance (specially for Bun), which is where I decided bringing this project to MySQL2.


Benefits for MySQL2 🐬

  • Resolves all issues related to "LRU is not a constructor" since it is fully compatible with both CJS and ESM environments, from Node.js 8 onwards using zero pollyfill (it works even in older browsers).
  • The same solution above also fixes the premature breaking change, requiring users to upgrade on Node.js 16.14 (dropping runtime versions isn't a problem, but it should be done in major releases)
  • When setMaxParserCache is used, MySQL2 overwrites/loses all previous cache (even for a higher size). After this PR, the cache is dynamically resized.

⚑️ This is minimally more performative and uses a little less CPU (but not such a significant difference).


Recognizing the importance of MySQL2, I will include some details about the reliability of the project in the next comment (please don't associate this with any kind of promotion).

Since this PR replaces lru-cache, I'll understand perfectly if you don't like it β€” please feel free to close it πŸ™‹πŸ»β€β™‚οΈ

wellwelwel avatar Aug 27 '24 11:08 wellwelwel

I would like to emphasize that lru-cache is the architecture basis of the proposed project, while methods/features and its functionalities are inspired by quick-lru 🀝

The real advantage is an extensive compatibility and the flexibility to dynamically resize the cache.

That said, "lru.min" was primarily optimized based on the use of MySQL2 and is 100% covered throughout E2E based tests:

  • Coverage

About performance, the benchmarks are created by comparing 1,000,000 runs through a maximum cache limit of 100,000, getting 333,333 caches and deleting 200,000 keys 10 consecutive times (isolated processes for each package), clearing the cache for each run.

  • CommonJS results example (In MySQL2's case)
# Time:
  lru.min:    242.86ms
  lru-cache:  264.30ms

# CPU:
  lru.min:    280118.00Β΅s
  lru-cache:  310327.20Β΅s

If someone wants to take a closer look, see how it happens here.


In practice, the main difference is how the cache is created (function instead of class):

- new LRU({ max })
+ createLRU({ max })

If there's an interest in merging this PR, that's the repository: lru.min.

@sidorares, I'm also completely open to include you as a maintainer (npm and GitHub) if you consider it ideal.

wellwelwel avatar Aug 27 '24 11:08 wellwelwel

Codecov Report

Attention: Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.13%. Comparing base (ddc299a) to head (babca6c). Report is 32 commits behind head on master.

Files with missing lines Patch % Lines
lib/parsers/parser_cache.js 66.66% 1 Missing :warning:
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2988   +/-   ##
=======================================
  Coverage   88.13%   88.13%           
=======================================
  Files          71       71           
  Lines       12889    12889           
  Branches     1352     1352           
=======================================
  Hits        11360    11360           
  Misses       1529     1529           
Flag Coverage Ξ”
compression-0 88.13% <88.88%> (ΓΈ)
compression-1 88.13% <88.88%> (ΓΈ)
tls-0 87.55% <88.88%> (ΓΈ)
tls-1 87.89% <88.88%> (ΓΈ)

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Aug 27 '24 11:08 codecov[bot]

I upgrade several hundred lambda function functions to a version of mysql2 that included this cache change and I observed no notable issues while testing, We have over 30+ developers / testers working in this environment so I am happy to leave for 24 hours to dogfood this.

So far no issues observed, even the deployment / CI phase went as expected without the LRU Constructor cache error.

robert-pitt-foodhub avatar Sep 09 '24 09:09 robert-pitt-foodhub

I upgrade several hundred lambda function functions to a version of mysql2 that included this cache change and I observed no notable issues while testing, We have over 30+ developers / testers working in this environment so I am happy to leave for 24 hours to dogfood this.

So far no issues observed, even the deployment / CI phase went as expected without the LRU Constructor cache error.

Thank you, @robert-pitt-foodhub 🀝

wellwelwel avatar Sep 09 '24 11:09 wellwelwel

Hey @wellwelwel, me again πŸ˜†, as I have upgraded to the latest version of mysql2 to resolve my hanging connections issue, I now am facing this issue and was looking for a solution, I have looked through the attached tickets and there's nothing that really is working for my setup, however there is some things that I have noticed.

  1. I am using an AWS CDK project, where during the building of my infrastructure, CDK will invoke build tools such as Esbuild to compile the typescript to JS, and they seem to be acting differently, for example:

Calling my build step invokes esbuild bundling, which I think leads to this error: Screenshot 2024-09-10 at 23 49 46

The error is simple, the LRU is not a constructor because the value of require("lru-cache").default is not a class, this is confirmed below when I executed the direct .js file using node binary (note I am using v18 for both build, compile and runtime), there is no Error throw, and this is because the LRU is imported correctly.

Screenshot 2024-09-10 at 23 52 06

I took at look at what the outputs of require('lru-cache') were for each scenario:

Node: Screenshot 2024-09-10 at 23 53 51

Via bundling process Screenshot 2024-09-11 at 00 05 13

Ok, so I can see that there is a difference in what's logged between each environment, however, before I was going to jump down the rabbit hole of the typescript, bundlers loaders etc, I checked out lru-cache's package.json, and noticed they are exporting a min folder that mapps require to the correct common js module format

Screenshot 2024-09-11 at 00 07 33

So I updated the import statements for both strings.js and parser_cache.js so that I import from lru-cache/min, such as const LRU = require('lru-cache/min').default;, and doing this fixes the error so it works on both scenarios I outlined above, via CDK and direct via node binary

there changelog also mentions this Screenshot 2024-09-11 at 00 14 53

My thoughts are, if you move to the /min import, everything should go back to normal and you can take some more time with the lru.min transition

robert-pitt-foodhub avatar Sep 10 '24 23:09 robert-pitt-foodhub

sorry for lack of input @wellwelwel , been offline for a week. Happy to see this merged!

sidorares avatar Sep 11 '24 02:09 sidorares

ow nice!!! now I can upgrade to version 3 finally!!! thanks

onhate avatar Sep 12 '24 14:09 onhate