uNetworking / uWebSockets.js

μWebSockets for Node.js back-ends :metal:
Apache License 2.0
7.86k stars 569 forks source link

HttpResponse.write(...) returns false, yet whole buffer was still written #726

Closed Zubnix closed 2 years ago

Zubnix commented 2 years ago

I'm currently debugging an issue where in a backpressure testcase, HttpResponse.write(...) returns false, indicating that backpressure has build-up. When later on the onWrite(...) method is called, I write the remainder of the buffer based on the write offset increment (based on the write offset before write and offset argument of onWrite).

However there appears to be some strange things happening. The offset is always 0 (for both old & new offset), causing the whole chunk to be written again. On the receiving end however, the chunk is received double, indicating that the original call to write was ok(?). I'm not sure if I'm doing something wrong, or that I'm triggering an edge case bug here?

I've based my implementation on the example 'video streaming' from this repo, however my use-case does not know the transfer length in advance like in the examle so I've made some adaptions.

Relevant code: https://github.com/udevbe/greenfield/blob/10c39cda79227ec2bf7d6c32ff191ef6ce647f19/compositor-proxy/src/AppController.ts#L164

test case: https://github.com/udevbe/greenfield/blob/10c39cda79227ec2bf7d6c32ff191ef6ce647f19/compositor-proxy/src/app.spec.ts#L325

ghost commented 2 years ago

Oh when you use write you shouldn't use onWritable offset. Write always buffers what you pass it.

So call write with small chunks, 500kb or so, until you get false, then continue in onWritable (but do not use offset, use your offset)

ghost commented 2 years ago

You have 4 calls; write, end, tryEnd and tryWrite. tryWrite is not implemented so you really only have 3 calls.

tryWrite and tryEnd will not buffer up, and you should then continue in onWritable with its offset.

end and write do buffer up, so when it returns false, it means you have backpressure and should continue in onWritable with the remaining data.

Zubnix commented 2 years ago

yes that made it work! Thanks!