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

fix: remove stream.emit('close') on stream end #2242

Closed
wants to merge 2 commits into from
Closed

fix: remove stream.emit('close') on stream end #2242

wants to merge 2 commits into from

Conversation

Ivan-Baranov
Copy link

On node v18+ stream.emit('close') produce Error: Premature close

at new NodeError (node:internal/errors:405:5)
at Readable.onclose (node:internal/streams/end-of-stream:154:30)
at Readable.emit (node:events:514:28)

@wellwelwel
Copy link
Collaborator

wellwelwel commented Oct 15, 2023

Hi, @Ivan-Baranov 🙋🏻‍♂️

The CI failure comes from stream/promises that is missing on Node 14.


Instead:

if (process.versions.node.split(".", 1)[0] <= 14) {
  stream.emit('close'); // notify readers that query has completed
}

An alternative could be to use stream/promises as callback, for example:

const { finished } = require('stream');

const rowsStreamForAwait = [];
function testStreamForAwait(callback) {
  const stream = connection.execute('SELECT * FROM announcements').stream();
  stream.on('data', row => {
    rowsStreamForAwait.push(row);
  });
  stream.on('end', () => {
    callback(null);
  });
  stream.on('error', err => {
    callback(err);
  });
}
testStreamForAwait(err => {
  if (err) {
    throw err;
  }
});


const rowsStreamFinished = [];
function testStreamFinished(callback) {
  const stream = connection.execute('SELECT * FROM announcements').stream();
  stream.on('result', row => {
    rowsStreamFinished.push(row);
  });
  finished(stream, err => {
    callback(err);
  });
}
testStreamFinished(err => {
  if (err) {
    throw err;
  }
});

process.on('exit', () => {
  assert.deepEqual(rows, rowsStreamForAwait);
  assert.deepEqual(rows, rowsStreamFinished);
});

Also (just a small tip), if you're familiar with Docker, you can test locally any version of MySQL and NodeJS with a simple docker-compose.yml 🐳

version: '3.9'

services:
  mysql2:
    image: node:14
    working_dir: /usr/app
    volumes:
      - ./path-to-mysql2:/usr/app:ro
    command: npm run test
    environment:
      MYSQL_HOST: db
      use-compression: 0
      use-tls: 0
    depends_on:
      db:
        condition: service_healthy

  db:
    image: mysql:5.7
    platform: linux/amd64
    restart: always
    environment:
      MYSQL_DATABASE: 'test'
      MYSQL_ALLOW_EMPTY_PASSWORD: 'yes'
    healthcheck:
      test: ['CMD', 'mysqladmin', 'ping', '-h', 'localhost', '-u', 'root']
      interval: 10s
      timeout: 5s
      retries: 5

Then, run docker compose up and wait for the last log as "exited with code 0" 🧙🏻✨

@Ivan-Baranov
Copy link
Author

I want to show error in node 18+, then used await finished(stream) or for await(let row of stream)

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

Successfully merging this pull request may close these issues.

2 participants