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

Race condition in test #238

Closed
LinusU opened this issue Oct 3, 2015 · 8 comments
Closed

Race condition in test #238

LinusU opened this issue Oct 3, 2015 · 8 comments

Comments

@LinusU
Copy link
Member

LinusU commented Oct 3, 2015

This seems to be very very rare, found it on Travis today.

$ npm test

> [email protected] test /home/travis/build/expressjs/multer
> standard && mocha



  Disk Storage
    ✓ should process parser/form-data POST request
    1) should process empty fields and an empty file
    ✓ should process multiple files
    ✓ should remove uploaded files on error
    ✓ should report error when directory doesn't exist

  Error Handling
    ✓ should respect parts limit
    ✓ should respect file size limit
    ✓ should respect file count limit
    ✓ should respect file key limit
    ✓ should respect field key limit
    ✓ should respect field value limit
    ✓ should respect field count limit
    ✓ should respect fields given
    ✓ should report errors from storage engines
    ✓ should report errors from busboy constructor
    ✓ should report errors from busboy parsing

  Expected files
    ✓ should reject single unexpected file
    ✓ should reject array of multiple files
    ✓ should reject overflowing arrays
    ✓ should accept files with expected fieldname
    ✓ should reject files with unexpected fieldname

  Express Integration
    ✓ should work with express error handling (42ms)
    ✓ should work when receiving error from fileFilter

  Fields
    ✓ should process multiple fields
    ✓ should process empty fields
    ✓ should not process non-multipart POST request
    ✓ should not process non-multipart GET request
    ✓ should handle basic keys
    ✓ should handle multiple values
    ✓ should handle deeper structure
    ✓ should handle sparse arrays
    ✓ should handle even deeper
    ✓ should handle such deep
    ✓ should handle merge behaviour
    ✓ should handle bad fields
    ✓ should convert arrays into objects

  File Filter
    ✓ should skip some files
    ✓ should report errors from fileFilter

  File ordering
    ✓ should present files in same order as they came

  Functionality
    ✓ should upload the file to the `dest` dir
    ✓ should rename the uploaded file
    ✓ should ensure all req.files values (single-file per field) point to an array
    ✓ should ensure all req.files values (multi-files per field) point to an array
    ✓ should rename the destination directory to a different directory

  Issue #232
    ✓ should report limit errors

  Memory Storage
    ✓ should process multipart/form-data POST request
    ✓ should process empty fields and an empty file
    ✓ should process multiple files

  Reuse Middleware
    ✓ should accept multiple requests (48ms)

  Select Field
    ✓ should select the first file with fieldname
    ✓ should select all files with fieldname

  Unicode
    ✓ should handle unicode filenames


  51 passing (412ms)
  1 failing

  1) Disk Storage should process empty fields and an empty file:
     Uncaught Error: ENOENT: no such file or directory, stat '/tmp/gvQUuo2Ne54/154080e1a19a860bba7db8c648f4358f'
      at Error (native)
      at Object.fs.statSync (fs.js:849:18)
      at Object.fileSize (test/_util.js:11:13)
      at test/disk-storage.js:81:25
      at Immediate.<anonymous> (test/_util.js:32:37)



npm ERR! Test failed.  See above for more details.

The command "npm test" exited with 1.

Done. Your build exited with 1.
@gireeshpunathil
Copy link
Contributor

@LinusU - are we seeing this still? (just trying to figure out whether we should keep it opened or not)

@Trott
Copy link
Contributor

Trott commented Oct 11, 2019

@LinusU - are we seeing this still? (just trying to figure out whether we should keep it opened or not)

@gireeshpunathil Definitely still seeing it. I've noticed it a fair amount in Node.js's ecosystem-testing utility CITGM. For example: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2040/nodes=ubuntu1604-64/testReport/junit/(root)/citgm/multer_v1_4_2/

I also just reproduced it locally on macOS Mojave by adding mocha's .only() to the relevant test. In other words, I changed:

it('should process empty fields and an empty file', function (done) {

...to be:

  it.only('should process empty fields and an empty file', function (done) {

...and then ran

$ while true; do
> node node_modules/.bin/mocha
>   if [[ "$?" -ne 0 ]]; then 
> break
> fi
> done

...and the failure seems to come up every twenty or thirty runs or so:

  1) Disk Storage should process empty fields and an empty file:
     Uncaught Error: ENOENT: no such file or directory, stat '/var/folders/7w/pzd4440x60d3gkc3cv7rj8lc0000gp/T/2MR49F8/a25a4ece9ac48e4fc21b525cd7edadd2'
      at Object.statSync (fs.js:926:3)
      at Object.fileSize (test/_util.js:11:13)
      at test/disk-storage.js:80:25
      at Immediate.<anonymous> (test/_util.js:32:37)
      at processImmediate (internal/timers.js:441:21)

@Trott
Copy link
Contributor

Trott commented Oct 11, 2019

Changing:

outStream.on('finish', function () {

...to:

      outStream.on('close', function () {

...makes it so I can't reproduce the test failure anymore.

Trott added a commit to Trott/multer that referenced this issue Oct 11, 2019
Change listened-for event on the output stream in storage/disk.js from
'finish' to 'close' to improve reliability. With 'finish', there is a
race condition such that sometimes the callback runs before the file
exists.

Refs: expressjs#238
@Trott

This comment has been minimized.

@ronag
Copy link

ronag commented Oct 11, 2019

Probably bug in Node. Proposed PR in core nodejs/node#29930.

ronag added a commit to nxtedition/node that referenced this issue Oct 11, 2019
'finish' could previously be emitted before the file has been
created when ending a write stream without having written any
data.

Refs: expressjs/multer#238
Trott pushed a commit to nodejs/node that referenced this issue Oct 13, 2019
'finish' could previously be emitted before the file has been
created when ending a write stream without having written any
data.

Refs: expressjs/multer#238

PR-URL: #29930
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
targos pushed a commit to nodejs/node that referenced this issue Oct 18, 2019
'finish' could previously be emitted before the file has been
created when ending a write stream without having written any
data.

Refs: expressjs/multer#238

PR-URL: #29930
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
@jonchurch
Copy link
Member

The referenced fix in Node core has landed, this should be tested again and determined if it's fixed for multer.

@Trott
Copy link
Contributor

Trott commented Dec 18, 2019

The referenced fix in Node core has landed, this should be tested again and determined if it's fixed for multer.

I haven't noticed the CITGM failure mentioned in #238 (comment) in a long time so I'd guess that any Node.js part of this is fixed on the master branch at least.

@gireeshpunathil
Copy link
Contributor

so closing as resolved; we can always re-open if necessary.

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

5 participants