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: wait for flush to finish #55

Closed
wants to merge 7 commits into from
Closed

fix: wait for flush to finish #55

wants to merge 7 commits into from

Conversation

ronag
Copy link
Contributor

@ronag ronag commented Dec 10, 2021

@@ -212,10 +212,9 @@ class ThreadStream extends EventEmitter {
throw new Error('the worker is ending')
}

if (this[kImpl].flushing && this[kImpl].buf.length + data.length >= MAX_STRING) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking for flushing here won't help us to avoid MAX_STRING length on buf.

try {
writeSync(this)
this[kImpl].flushing = true
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't make sense?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It did. There is a flush scheduled in nextTick where this was correctly reset.

index.js Outdated Show resolved Hide resolved
@ronag
Copy link
Contributor Author

ronag commented Dec 10, 2021

@mcollina PTAL

@coveralls
Copy link

coveralls commented Dec 10, 2021

Pull Request Test Coverage Report for Build 1564728806

  • 15 of 18 (83.33%) changed or added relevant lines in 1 file are covered.
  • 7 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-1.1%) to 82.974%

Changes Missing Coverage Covered Lines Changed/Added Lines %
index.js 15 18 83.33%
Files with Coverage Reduction New Missed Lines %
index.js 7 84.26%
Totals Coverage Status
Change from base Build 1558216733: -1.1%
Covered Lines: 282
Relevant Lines: 332

💛 - Coveralls

}
process.nextTick(cb)
})
writeSync(this) // TODO (fix): Make this async somehow.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#56

index.js Show resolved Hide resolved
@ronag
Copy link
Contributor Author

ronag commented Dec 10, 2021

@mcollina I added a test that fails on main

@mcollina
Copy link
Member

I would need to look into this with a couple of free hours. This logic is quite tricky. I'll get to it next week.

@mcollina
Copy link
Member

I have tried running the attached test to this PR and it passes on main.

@mcollina
Copy link
Member

I was able to repro with:

'use strict'

const t = require('tap')
const { join } = require('path')
const { file } = require('./helper')
const { createReadStream } = require('fs')
const ThreadStream = require('..')
const buffer = require('buffer')

const MAX_STRING = buffer.constants.MAX_STRING_LENGTH

t.plan(1)

const dest = file()
const stream = new ThreadStream({
  filename: join(__dirname, 'to-file.js'),
  workerData: { dest },
  sync: false
})

stream.on('close', async () => {
  t.comment('close emitted')
  let buf
  for await (const chunk of createReadStream(dest)) {
    buf = chunk
  }
  t.equal('asd', buf.toString().slice(-3))
})

stream.on('open', () => {
  t.comment('open emitted')
  stream.write('a'.repeat(MAX_STRING - 2))
  stream.write('asd')
  stream.end()
})

@mcollina
Copy link
Member

My bad, I made a mistake in the previous example. We emit 'ready' and not 'open'.

I cannot reproduce the problem using the attached test.

@ronag
Copy link
Contributor Author

ronag commented Dec 14, 2021

You are right I had not updated my local main.

@ronag ronag closed this Dec 14, 2021
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.

3 participants