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 handling of FormData errors #1468

Merged
merged 1 commit into from
Mar 14, 2019
Merged

Fix handling of FormData errors #1468

merged 1 commit into from
Mar 14, 2019

Conversation

bajtos
Copy link
Contributor

@bajtos bajtos commented Mar 12, 2019

This patch is fixing regression introduced by 6670a0f, see #1466 for detailed discussion.

/cc @kornelski @lapo-luchini

I have verified the fix by running the following test manually. Please advise me how to capture these test cases in your automated test suite.

The first two use cases are already captured in the existing test suite, so I updated the existing tests to match the new behavior. Then I added two new tests to verify what happens when two errors are triggered by a single request (the last case below).

// remote host is listening, attaching a missing file
// PROMISE VARIANT
require('.').post('http://example.com/does-not-exist')
.field({a:1,b:2})
.attach('c', 'does-not-exists.txt')
//.on('error', e => console.log('ERR', e.message))
.then(ok => console.log('THEN'))
.catch(e => console.log('CATCH', e.message));

// remote host is listening, attaching a missing file
// CALLBACK VARIANT
require('.').post('http://example.com/does-not-exist')
  .field({a:1,b:2})
  .attach('c', 'does-not-exists.txt')
  //.on('error', e => console.log('ERR', e.message))
  .end((err, res) => {
    if (err) console.log('CATCH', err && err.message);
    else console.log('THEN');
  });

// remote host is not listening
// two errors are triggered: cannot read the file, cannot connect to 127.0.0.1:80
require('.').post('http://127.0.0.1/test')
.field({a:1,b:2})
.attach('c', 'does-not-exists.txt')
//.on('error', e => console.log('ERR', e.message))
.then(ok => console.log('THEN'))
.catch(e => console.log('CATCH', e.message));

When a file cannot be uploaded (e.g. becase it does not exist), report
the error via callback/promise.

Before this change, FormData errors were reported via "error" event
on the request. In promise mode, such error was caught by `.then()`
handler in a way that shadowed the actual error reported by `.end()`.
In callback mode, such error was not caught at all and crashed the
process on an unhandled error.

With this change in place, FormData errors finish the request and pass
the error directly to the callback.
@bajtos
Copy link
Contributor Author

bajtos commented Mar 14, 2019

I have added two new tests to verify that ECONNRESET errors take precedence over ENOENT errors, I think we have sufficient test coverage now. I also fixed npm test failures and improved the commit message to better describe what is being changed.

As far as I am concerned, this pull request is ready for final review and merging.

@kornelski Could you please take a look? I understand you might not have time to do it right now, in which case could you please let me know when you will have time to process my pull request?

@kornelski kornelski merged commit 076e55a into ladjs:master Mar 14, 2019
@bajtos bajtos deleted the fix/formdata-error branch March 15, 2019 13:21
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