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

🐛 streaming reads should emit the dataset number for each dataset #2496

Conversation

fluffynuts
Copy link
Contributor

When upgrading from mysql to mysql2, to support authentication updates on our mysql server(s), I found that my code around streaming, particularly when streams contain multiple resultsets, wasn't working properly because the resultset index wasn't being passed through as it is in mysql. This PR aims to resolve that.

@wellwelwel
Copy link
Collaborator

wellwelwel commented Mar 13, 2024

Thanks @fluffynuts, 🙋🏻‍♂️

By including your test into test/integration directory, it will be performed when running the npm run test.

Then, it's not necessary to add the test-me in the package.json 🧙🏻


To test a specific file, you can use the FILTER environment variable:

FILTER=test-multi-result-streaming npm run test

Also, to keep good practices, there are some small suggestions:

Change from

const { createConnection } = require('../common.test.cjs');
(async function() {
  'use strict';

  const { assert } = require('poku');

  // ...

To

'use strict';

const { assert } = require('poku');
const { createConnection } = require('../common.test.cjs');

(async () => {
  // ...

@fluffynuts
Copy link
Contributor Author

thanks @wellwelwel

I didn't know how to filter poku tests (this is the first time I've seen poku, tbh), so that was supposed to be a temporary npm script until I had a passing test (but it turned out I could run through webstorm anyway, so...) - anyways, it's removed and the requested changes have been made

@wellwelwel
Copy link
Collaborator

@fluffynuts, sorry, I hadn't noticed something before:

You need to name your test file as *.test.cjs in the test/integration/** path 🙋🏻‍♂️

- test/integration/test-multi-result-streaming.cjs
+ test/integration/test-multi-result-streaming.test.cjs

@codecov-commenter
Copy link

codecov-commenter commented Mar 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.32%. Comparing base (9edfd72) to head (f5bb633).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2496   +/-   ##
=======================================
  Coverage   90.32%   90.32%           
=======================================
  Files          71       71           
  Lines       15727    15727           
  Branches     1339     1340    +1     
=======================================
  Hits        14206    14206           
  Misses       1521     1521           
Flag Coverage Δ
compression-0 90.32% <100.00%> (ø)
compression-1 90.32% <100.00%> (ø)
tls-0 89.85% <100.00%> (ø)
tls-1 90.15% <100.00%> (ø)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wellwelwel wellwelwel added the mysqljs-mysql-incompatibilities Previously: feligxe-mysql-incompatibilities label Mar 13, 2024
@wellwelwel
Copy link
Collaborator

wellwelwel commented Mar 13, 2024

@fluffynuts, for some reason the test/integration/test-multi-result-streaming.test.cjs:54:12 brings a random result:

  • Sometimes it will just work.
  • Then, sometimes it just won't.

Can you check it?


You can also test it by using the native Node.js command:

node test/integration/test-multi-result-streaming.test.cjs

I don't think that it's related to this issue, but you don't need to try-catch the assert:

-  try {
    assert.equal(captured1.length, 1);
    assert.equal(captured2.length, 1);
    assert.deepEqual(captured1[0], compare1[0][0]);
    assert.deepEqual(captured2[0], compare2[0][0]);
-    process.exit(0);
-  } catch (e) {
-    console.error(e);
-    process.exit(1);
-  }

🙋🏻‍♂️

@wellwelwel
Copy link
Collaborator

wellwelwel commented Mar 13, 2024

About the lint error, you can just run npx prettier --write ./test/integration/test-multi-result-streaming.test.cjs 🧙🏻

@fluffynuts
Copy link
Contributor Author

fluffynuts commented Mar 14, 2024

@wellwelwel I've finally found a fail (sorry, it's right at the end, and searching for my test name was coming up blank)

I've literally just run this test more than 100x without issue, so perhaps it's specific to node 21 and / or something else. I'll investigate, but I see a lot of errors like Warning: got packets out of order. Expected 27 but received 2 and I don't even know the source of that error - searching this project doesn't turn it up. I'd appreciate any help on this as I'm not a full-time node dev and I don't always have time to noodle about with non-lts node variants. Like I say, I'll have a look, but I don't know if I'll get anywhere.

I also notice that the errors only occur with node >= 20 and SSL disabled for mysql 8.0.33, with errors about packets. I'm wondering if there's an SSL-related issue in there somewhere.

Edit: using mysql 8.0.35 on windows doesn't repro the issue at all, with node 20 or 21.

@fluffynuts
Copy link
Contributor Author

fluffynuts commented Mar 14, 2024

I'm wondering if an update between 8.0.33 and 8.0.35 fixed something. There was an SSL-related update in 8.0.34 which affects systems shipping their own SSL libraries (eg good linux distros):
image

would it be possible to update the 8.0.33 matrix target to 8.0.35 to test the idea?

@wellwelwel
Copy link
Collaborator

wellwelwel commented Mar 14, 2024

I don't believe that it's related to a specific Node and MySQL version.
For example, this time the previous issue happened for Node.js 14, 16, also for MySQL 8.0.33 and SSL 0 and 1 (same for compression).

This really does seem to happen at random.
Apart from a real debugging, the only similarity is that errors only occur on Linux, not on Windows and OSX.


About the "Warning: got packets...", it's not causing the error 🙋🏻‍♂️
The problem occurs here:

assert.deepEqual(captured2[0], compare2[0][0]);

Also, you don't need to force the process exit code 🙋🏻‍♂️

- process.exit(0);

I'd appreciate any help on this

I can try to do some debugging at the weekend, but in any case I've added a help wanted label 🤝

@wellwelwel
Copy link
Collaborator

Hey @fluffynuts, there's a CI exclusive for MySQL versions:

mysql-version:
[
'mysql:5.7',
'mysql:8.0.18',
'mysql:8.0.22',
'mysql:8.0.33',
'mysql:latest',
]

In that case, we already use the mysql:latest 🙋🏻‍♂️

@fluffynuts fluffynuts force-pushed the fix/streaming-interface-should-provide-dataset-index branch from 4ea147a to dfd19e8 Compare March 14, 2024 08:28
@wellwelwel
Copy link
Collaborator

wellwelwel commented Mar 14, 2024

enable GHA for this branch, hopefully

@fluffynuts, you can submit a PR to your fork to test independently, it will enable all the CI for pull request events in your own fork 🧙🏻

@fluffynuts
Copy link
Contributor Author

I've now run this test around 400x with and without TLS on an unbuntu 22.04.4 machine with mysql 8.0.36 (just installed) - no errors. I'm going to try bumping the mysql version in the github action file.

Hey @fluffynuts, there's a CI exclusive for MySQL versions:

mysql-version:
[
'mysql:5.7',
'mysql:8.0.18',
'mysql:8.0.22',
'mysql:8.0.33',
'mysql:latest',
]

In that case, we already use the mysql:latest 🙋🏻‍♂️

cool, but that's not the fixture that's failing - linux-ci is failing on version 8.0.33. I've tried upping that to 8.0.36, and I get more failures (node 14 as well!), but all of them have this junk in their output:

image

and I haven't seen this in about 1000x runs on an ubuntu vm in virtualbox.

@wellwelwel
Copy link
Collaborator

wellwelwel commented Mar 14, 2024

Can you try using a Docker container?

Usually, I use the node:${version}-alpine images, but for this case, I think using node:${version} is better to test it ensuring the Ubuntu environment.

If you don't know how to reproduce these tests on Docker, check the #2493 (comment) 🙋🏻‍♂️


As before, the error occurs on assert (assert.deepEqual(captured2[0], compare2[0][0])), these warnings aren't causing the CI failure.


Also, forcing the process exit code to 0 can hinder the debugging process:

- process.exit(0);

@fluffynuts
Copy link
Contributor Author

fluffynuts commented Mar 14, 2024

after running the full suite in the same ubuntu vm, I am seeing the same "Warning: got packets out of order", but those tests don't fail, so that's odd.

I'll try to get docker running

As before, the error occurs on assert (assert.deepEqual(captured2[0], compare2[0][0])), these warnings aren't causing the CI failure.

I'd agree that warnings (in themselves) don't cause test failures. I'd also postulate that if network comms to the mysql server are not running correctly, writing code that assumes that it can query data from the database without error may well be in error. I only saw these warnings on the ubuntu GHA hosts, until right now when I ran the full set of tests (but I only see those warnings on other tests which aren't failing). What I've read about that warning suggests that data can be lost during transmission when that occurs - which would explain the failure.

@wellwelwel
Copy link
Collaborator

wellwelwel commented Mar 14, 2024

Can you please revert the changes in the .github/workflows/ci-linux.yml?

@wellwelwel
Copy link
Collaborator

wellwelwel commented Mar 14, 2024

Can you please revert the changes in the .github/workflows/ci-linux.yml?

Sorry about that, but I don't see an advantage to change MySQL version on Node / Bun CI.

It should fit more in CI - MySQL, but since we've already gotten the error and it's happening in different cases, I don't see why we should include that version in the matrix at this moment.

@fluffynuts
Copy link
Contributor Author

Can you please revert the changes in the .github/workflows/ci-linux.yml?

Sorry about that, but I don't see an advantage to change MySQL version on Node / Bun CI.

It should fit more in CI - MySQL, but since we've already gotten the error and it's happening in different cases, I don't see why we should include that version in the matrix at this moment.

similarly, there's no disadvantage; however I've reverted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mysqljs-mysql-incompatibilities Previously: feligxe-mysql-incompatibilities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants