1035428 - Writable callback for TURN TCP is not rescheduled after writing from queue

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect, P2)

Description

[Byron Campen [:bwc]]

If at any point this function gets R_WOULDBLOCK:

http://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/stun/nr_socket_buffered_stun.c#409

We do not reschedule the writable callback, which means that the queue sits unserviced until we attempt to write some new bytes, here:

http://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/stun/nr_socket_buffered_stun.c#399

If this happens during the setup or connectivity establishment phase with TURN TCP, we end up dead in the water, because there are no retransmissions that will cause nr_socket_buffered_stun_sendto to be called.

Comment 1

[Byron Campen [:bwc]]

Attachment: Re-register writeable callback after partially servicing the send queue.

Comment 2

[Nils Ohlmeier [:drno]]

Comment on attachment 8451937 [details] [diff] [review]
Re-register writeable callback after partially servicing the send queue.

Review of attachment 8451937 [details] [diff] [review]:
-----------------------------------------------------------------

I think the ABORT() usage is worth fixing.
But in general LGTM.

::: media/mtransport/third_party/nICEr/src/stun/nr_socket_buffered_stun.c
@@ +446,5 @@
> +    // Re-arm
> +    NR_SOCKET fd;
> +
> +    if ((r=nr_socket_getfd(sock->inner, &fd)))
> +      ABORT(r);

Where is this ABORT() going to jump to?

Comment 3

[Byron Campen [:bwc]]

I think it will hop back up to the abort: label and do the right thing, but that's just weird enough that I'll change the code.

Comment 4

[Byron Campen [:bwc]]

Attachment: Re-register writeable callback after partially servicing the send queue.

Incorporate feedback.

Comment 5

[Byron Campen [:bwc]]

Attachment: Re-register writeable callback after partially servicing the send queue.

Incorporate feedback.

Comment 6

[Byron Campen [:bwc]]

Attachment: Re-register writeable callback after partially servicing the send queue.

Consolidate some code.

Comment 7

[Byron Campen [:bwc]]

Attachment: Re-register writeable callback after partially servicing the send queue.

Actually, I can't consolidate that.

Comment 8

[Byron Campen [:bwc]]

Attachment: Re-register writeable callback after partially servicing the send queue.

Move this function.

Comment 9

[Byron Campen [:bwc]]

https://tbpl.mozilla.org/?tree=Try&rev=f5f18b740380

Comment 10

[Byron Campen [:bwc]]

Comment on attachment 8481479 [details] [diff] [review]
Re-register writeable callback after partially servicing the send queue.

Review of attachment 8481479 [details] [diff] [review]:
-----------------------------------------------------------------

This changed enough that I'd like another pass.

Comment 11

[Nils Ohlmeier [:drno]]

Comment on attachment 8481479 [details] [diff] [review]
Re-register writeable callback after partially servicing the send queue.

Review of attachment 8481479 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

Comment 12

[Maire Reavy [:mreavy]]

Byron  - can we land this?

Comment 13

[Byron Campen [:bwc]]

Attachment: Re-register writeable callback after partially servicing the send queue

Comment 14

[Byron Campen [:bwc]]

Comment on attachment 8627721 [details] [diff] [review]
Re-register writeable callback after partially servicing the send queue

Review of attachment 8627721 [details] [diff] [review]:
-----------------------------------------------------------------

Unrotted. Carry forward r=drno.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=202e2ea57397

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e9e66c55eaa5

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e9e66c55eaa5