Make uv_udp_send send data immediately if possible #1358
Conversation
| @@ -422,8 +401,15 @@ int uv__udp_send(uv_udp_send_t* req, | |||
| if (err) | |||
| return err; | |||
|
|
|||
| uv__req_init(handle->loop, req, UV_UDP_SEND); | |||
| /* It's legal for write_queue_size > 0 even when the write_queue is empty; | |||
|
Please yes. UDP is important, really. Not so much as TCP (and its loved HTTP which dominates the world and will replace "Internet") but, somehow, UDP may be important. |
|
Thanks for the comments, @ibc. I wanted to put it up for review quickly in case you guys had some spare cycles to review the code. Now, this patchset includes some required minor changes to how UDP handles operate internally, which actually make them a bit more similar to how streams behave:
An interesting note: the change I had to make in test-watcher-cross-stop ilustrates the usefulness of this patch. On Linux, all send requests are completed immediately, on that test. |
Nobody said otherwise. Features are developed if/when there is need, curiosity, interest or patches, but there is only so much time. Discussions and patches like what @txdv you and I have done for try_send really motivate me to improve UDP, so here we are :-) |
|
I'm reviewing wait a bit. |
Thanks! No rush, we are not in a hurry. |
|
@txdv force pushed. Now all the requests that will never complete are added to que completed queue and run in order. This actually makes the code more concise, as there is a single place where the completed queue is processed, req-> status will have the correct value (ECANCELLED), and I added a couple of asserts about queue_size/count, since they should be 0 after all callbacks have been run. |
|
More delitions than additions and a feature more. One central place to call the the send_cb makes it more concise, yes. One question though, why are we calling the send_cb on success with 0 though? Don't we want to pass the amount we have written? |
I didn't change that. I guess the reason is that in UDP you either send the whole datagram or you don't. But we could get rid of the if and always return req->status, which has the number of bytes written. |
Hum, I cannot reproduce that on my Linux machine. I'll go through the code and test it on OSX as well a bit later. |
|
@saghul It was my changed that made it fail, It is not failing. Node used to return the buffer.length (which was faulty, because we could send less). nodejs/node-v0.x-archive@a382c9a97 look at dgram.js Now it returns the sent size. Maybe we should get rid of the if in libuv and just pass status. It would be 'consistent' with the write function I guess? Do stream_write cbs get a callback with 0 or the size of what they have written? |
|
@txdv they get 0, because the callback is not called until all data has been sent. So it's already consistent. I could update the comment, though. |
|
Does the uv_write_cb in uv_write get called with 0 as well? |
Yes. |
|
@indutny, this one is ready to land IMHO, can you PTAL? |
|
I already merged the last patch I had in this set (377bf68). It was unrelated and almost all UDP tests were failing on Windows without it. |
|
yes, my mistake, should have been in my commit |
|
No problem! |
|
@indutny, any further comments? |
It's possible that recv_cb_called is bigger than the number of sockets, because if all sends succeed the recv callback is called twice: once with the actual data, and another time with 0.
|
Thanks guys, landed! |
dgram#send callback was changed synchronously. The PR-URL is here joyent/libuv#1358 This commit is temporary fix until libuv issue is resolved. libuv/libuv#301 PR-URL: #1313 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
dgram#send callback was changed synchronously. The PR-URL is here joyent/libuv#1358 This commit is temporary fix until libuv issue is resolved. libuv/libuv#301 PR-URL: nodejs#1313 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
Please review with care.
/cc @indutny @txdv @ibc