dgram: add maxSendQueueSize socket option #10902
Conversation
nodejs-github-bot
added
the
dgram
cjihrig
added
the
semver-minor
|
What problem is this solving that needs to be solved at this level? Would there be a followon PR for TCP and file? What is special about the accumulation of async actions for dgram as opposed to the others? Would exposing the bytes and asyncs queued as a number and letting user-land implement policies work? |
I don't think so. I don't think they need them.
As far as I can tell, if DNS is down, dgram send operations accumulate indefinitely. I haven't seen that behavior with net and files.
It would, but I would compare this to the |
|
cc @Raynos |
|
net does not have this problem because there is a dgram has this problem because it's not a |
This commit adds the maxSendQueueSize option to dgram sockets. When used, this option prevents the socket's send queue from growing without an upper bound.
cjihrig
changed the title
|
Removed the |
|
ping @nodejs/collaborators |
|
The technical side looks fine to me but I don't know if I agree it's the best approach. A hard limit that is an error to hit is rather inflexible. Strawman counterproposal: what if .send() and .sendto() indicated whether the data was sent or queued so the caller could act accordingly? |
|
@bnoordhuis I think implementing backpressure for a dgram socket is not a good idea because it creates too much similarity between dgram and net, and there is already too much. TCP and UDP are two different things, and UDP does not implement any type of flow control. @cjihrig I do not like the error either. I think we should just call the callback with an error, and only if a callback is not present BTW, I think we should set this default value to an arbitrary number like 65536, or maybe maintaining the actual length in bytes of the queue, and set it to some amount of MB. That would be a separate change with semver-major. |
|
I took a stab at a different approach in cjihrig@015e3f9, which doesn't expose the implementation detail that there is a queue at all. PTAL In the current situation, DNS failures had no effect on the queue (we were only handling |
|
@cjihrig I like the second solution much more, and it is inline with how UDP works. 👍 |
| self.emit('error', new Error('Maximum send queue size exceeded')); | ||
| return; | ||
| } | ||
|
|
||
| self._queue.push(toEnqueue); | ||
| return; |
There was a problem hiding this comment.
I think its safe to remove this.
When send() triggers an implicit bind, the send operation is added to an internal queue. If a DNS error occurs during the bind, there is currently no mechanism for clearing the queue other than sending more data. If DNS errors keep occurring, the queue will continue to grow with no upper bound. This commit reports errors with implicit binds, and clears the queue. This should be fine, given the nature of UDP. Refs: nodejs/node-v0.x-archive#8705 Refs: nodejs#10902 PR-URL: nodejs#11036 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>
This commit adds a
maxSendQueueSizeoption todgramsockets. When used, this option prevents the socket's send queue from growing without an upper bound.Note that the send queue contains bound
send()functions. This option limits the number of outstanding sends, not the actual size of the data being sent.This is a WIP because docs and tests are still needed. I wanted to get feedback first.
Fixes: nodejs/node-v0.x-archive#8705
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
dgram