Dup of #3059?

@tflanagan it is not a dup.

Why don't we move this:

Should be way simpler than the current version of PR.

Also, please run make test to ensure that it works, and this change requires a test case as well.

Thanks for figuring it out!

I have a question, though... Does it really change anything when applied with #3059 ?

One more comment: I think if you'll make it work as I suggested in first comment - it looks like there should not be any need in #3059 anymore, as the finish event will be guaranteed to fire after prefinish. Do you think this is a correct statement?

cc @trevnorris @nodejs/collaborators

You are right #3059 is solved by this. If suggested block is moved into first if after it, it will continue in last else and crash on trying to buffer empty data... we need to return true in this case but not invoke callback... is it right?

@mareksrom why will it crash? It doesn't look like it will do it. Returning true does not make sense to me, we are not going to send anything anyway.

@mareksrom one more question: why is it better than #3059 ? What else is it doing? I like the minimalistic nature of this PR, but just want to see if it fixes more issues than #3059. So far it looks like it isn't.

I'm not sure, but I had it this way and maybe it crashed on writing nothing to connection or I don't know, I can try it again, maybe it was another problem... I'm going to try it...

#3059 does not work... it only calls prefinish before finish, but what does prefinish? problem is that prefinish and finish should be called after socket was assigned and data were flushed, #3059 calls prefinish with finish callback in time it does not have even connection assigned...

@indutny your proposal to move empty data to first if is ok... let's do it this way...

There are style issues, but I like the approach.

@mareksrom any news? ;)

@indurny what news? modification you proposed works and it can be this way, question is if there has to be all the block with check for data.length===0, without it it will also work, just less effective (useless empty write) that will also happen if we change it according to your proposal...

@mareksrom I think I have it working locally. Btw, is the email on your profile correct?

@indutny ...

@mareksrom I mean github profile.

@mareksrom argh, I meant the one that you are using for commits: https://github.com/mareksrom/node/commit/192b28bf2fc688f3da9ad0caa0ecbb84efaf3330.patch

@mareksrom anyway, may I ask you to contact me by email? It is available on my github's profile page: https://github.com/indutny

yes it is...

This isn't related to 11e4249, right?

@Fishrock123 It is.

@indutny Any news on this issue?

Will this be merged or are we still awaiting changes?

we'll make sure the fix gets in to 4.1.2 but that's slated for Monday so we have time to resolve outstanding concerns that this has been properly solved

Thanks for the update, @rvagg!

Fedor's modified fix(es) landed in 342c3a1...a4fa51c and released as 4.1.2, see: https://nodejs.org/en/