Skip to content

res.sendFile does not invoke callback for aborted requests.#2305

Closed
manuelstofer wants to merge 15 commits intoexpressjs:masterfrom
manuelstofer:fix-sendfile
Closed

res.sendFile does not invoke callback for aborted requests.#2305
manuelstofer wants to merge 15 commits intoexpressjs:masterfrom
manuelstofer:fix-sendfile

Conversation

@manuelstofer
Copy link

I added two currently failing tests for cases where sendFile fails to invoke the callback.

@manuelstofer
Copy link
Author

It's debatable if aborts are errors.

The current documentation is:

The callback fn(err) is invoked when the transfer is complete or when an error occurs

Not invoking the callback at all would actually be correct if aborts are not counted as errors. But it's unexpected behaviour.

I agree that an abort is not necessarily an error on http request level (for requests without body). But i think it is an error in sendFiles context, because it prevents it from "completing the transfer".

So I'm not sure if i should pass an error to the callback if the request is aborted.

@manuelstofer
Copy link
Author

But anyway, the purpose of this pull request is not to change the thing about the aborts, so i removed the abort errors. This pull request just ensures that the callback is always invoked.

@dougwilson dougwilson added the 4.x label Aug 18, 2014
@dougwilson dougwilson self-assigned this Aug 18, 2014
@dougwilson dougwilson added the 3.x label Aug 18, 2014
@manuelstofer
Copy link
Author

@dougwilson
There were commits removed from master? Should i removed them from the pull request too?

@dougwilson
Copy link
Contributor

There were commits removed from master?

Oh, so the repo was not correctly up to date for a while (it was my fault :( ). That was it getting back in sync.

Should i removed them from the pull request too?

Up to you, but it's not necessary.

dougwilson added a commit that referenced this pull request Aug 21, 2014
@dougwilson dougwilson mentioned this pull request Aug 21, 2014
4 tasks
@dougwilson
Copy link
Contributor

@manuelstofer can you try out the 4.9 branch to see if your issue has been resolved? npm install strongloop/express#4.9 will grab the code to test.

@manuelstofer
Copy link
Author

Works great!

@dougwilson
Copy link
Contributor

Awesome, thanks for the confirmation, @manuelstofer , I appreciate it!

dougwilson added a commit that referenced this pull request Aug 24, 2014
dougwilson added a commit that referenced this pull request Sep 9, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants