[http2] Force complete if all data received

[asilvas]

This was a significant bug for us resulting in requests failing intermittently. Very difficult to reproduce, which is why I didn't create a special test -- perhaps someone more familiar could. Looking through node's http2 issues, not clear if this is something that will ever be resolved in their stack. This workaround should be reliable and safe to do in any scenario.

[dougwilson]

Thank you for this. This does sound like a Node.js bug, but that doesn't mean we cannot land a "workaround" for it. Please add a comment to the Node.js issue that this is working around (and if you have not raised one yet, please do and link it back here).

In addition, we need to get new tests added here to accept it that test this change, especially if this is a workaround so we know when/if it's no longer needed or a Node.js update breaks the workaround.

Thank you!

[dougwilson]

If you need help creating a test, I can certainly do so, but there is not enough information provided here for me to even know where to begin, so maybe some kind of deeper explanation on what is happening or the link to the Node.js bug report about the missing end event would help me help you out there.

[dougwilson]

P.S. isn't the Content-Length request header still optional in http/2 like it is in http/1? If so I'm not sure that this will actually fix the issue, as requests would still come in where the length is unknown? Or does the issue only ever happen on the http/2 requests where there is a Content-Length but the end is always correctly emitted on requests without the header?

[asilvas]

nodejs/node#29529

Regarding what to test. Really just need to leave the stream open to prevent end from firing, but streaming the same number of bytes provided in the content-length header which will trigger onEnd. This will allow you to verify the callback is invoked successfully even though the stream has not yet ended. FYI existing tests are already triggering this pathway, the only difference is the stream is also ending which is benign since complete is true.

[asilvas]

Or does the issue only ever happen on the http/2 requests where there is a Content-Length but the end is always correctly emitted on requests without the header?

It's true this will only resolve requests that have content-length, but it isn't clear if this is also an issue for http2 requests w/o the header. It's very difficult to reproduce.

[dougwilson]

nodejs/node#29529

This linked issues seems to be about something completely different than the end missing, except for one random comment about it that it seems everyone ignored. Is that the only place the missing end event was mentioned? They may not even know about that particular case.

It's true this will only resolve requests that have content-length, but it isn't clear if this is also an issue for http2 requests w/o the header. It's very difficult to reproduce.

We need to figure this out prior to landing, as if this is a workaround, we need to have a good grasp on what to explain to users in the readme for example that if they are passing in x type of stream they need to pass in this header's value but it many still hang under y condition, so it's transparent about what this module is going to "fix" and what it is not going to.

Regarding what to test. Really just need to leave the stream open to prevent end from firing, but streaming the same number of bytes provided in the content-length header which will trigger onEnd. This will allow you to verify the callback is invoked successfully even though the stream has not yet ended.

Yes, this would be the basic test, but we also need a test with the actual "broken" implementation as well. Otherwise we don't know when/if it gets fixed, or they make a change that breaks the workaround.

[dougwilson]

P.S. this is a major breaking change since this module detects with a stream emits more data than expected and with this change it will no longer treat that as an error condition.

[asilvas]

Yes, this would be the basic test, but we also need a test with the actual "broken" implementation as well. Otherwise we don't know when/if it gets fixed, or they make a change that breaks the workaround.

Knowing how difficult this is to reproduce, I'm fairly certain we're not going to be successful forcing the actual broken implementation. I guess we'll have to go a different path if this is what you require.

[dougwilson]

Like I said, I am 100% open to help make said test, but there isn't really enough information here on where I shoudl even begin to look. Maybe if you can even provide the complete code setup that has the issue, but say you're not sure how to make it happen would be at least some kind of starting point for me to help.

[asilvas]

This is as close as we've gotten to reproducing the problem. nodejs/node#31309

[dougwilson]

To follow up on this, it was indeed a bug in Node.js that has been since fixed 👍