HTTP outgoing queue bug fix (#2639) #3068
Conversation
This change fix bug of incorrect http outgoing queue which caused crash on assert. I think there should be some other changes to make ending of response more transparent, this change only fix the bug.
|
Dup of #3059? |
|
@tflanagan it is not a dup. |
|
Why don't we move this: Lines 136 to 140 in 6192c98 if clause after it? So that callback won't be invoked immediately for the empty write if there is no assigned connection.
Should be way simpler than the current version of PR. Also, please run 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 |
|
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 |
|
@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... |
|
@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? ;) |
| if (data.length === 0) { | ||
| if (typeof callback === 'function') | ||
| //callback cannot be called (finish) when the connection is not assigned | ||
| if (typeof callback === 'function' && this.connection) |
There was a problem hiding this comment.
connection is already asserted above.
There was a problem hiding this comment.
We have a better fix, feel free to skip this PR.
There was a problem hiding this comment.
We have a better fix, feel free to skip this PR.
Are you going to open another PR?
|
@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/ |
This change fix bug of incorrect http outgoing queue which caused crash on assert. I think there should be some other changes to make ending of response more transparent, this change only fix the bug.