Add blocking write mode for association #356
Conversation
cnderrauber
requested review from
davidzhao,
edaniels,
enobufs,
Sean-Der and
boks1971
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #356 +/- ##
==========================================
- Coverage 81.66% 81.58% -0.08%
==========================================
Files 51 51
Lines 4341 4383 +42
==========================================
+ Hits 3545 3576 +31
- Misses 654 663 +9
- Partials 142 144 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Block write is useful for cases like sfu to forward reliab message between clients and detach the data channel to use it as normal golang connections.
cnderrauber
force-pushed
the
block_write
branch
from
4fc02c4 to
99301e5
Compare
|
Love this feature! Great idea putting it behind a SettingEngine option. Would it be possible to implement this by watching/peeking at It would allow us to keep the internals a lot simpler. |
It will reduce the throughput if we watch the pending/inflight bytes to become zero that every write needs to wait for previous one to be acked by remote peer, it is also not identical to the net.conn's write method which returns after copy the data to the socket buffer. |
| a.lock.Unlock() | ||
| select { | ||
| case <-ctx.Done(): | ||
| return ctx.Err() |
There was a problem hiding this comment.
how should this error be handled? does this bubble up to the app level or it is all handled internally? Am curious about how this affects reliable transmission mode and if app need to retry send?
There was a problem hiding this comment.
It will return to upper application, when WriteSCTP returns error, it means the data has not been sent (incorrect state or deadline timeout) so no retransmission here, the retransmission only works for sent messages (write operation succeeded).
There was a problem hiding this comment.
got it, wondering if the application is supposed to try Write again?
There was a problem hiding this comment.
It should do that if blocking mode is enabled, like other connections. The default behavior is still unblock mode
|
@cnderrauber @boks1971 Push back if you disagree, but I think we should have This would be a deeper change. I think it will be easier to maintain/follow. As more gets added to the code base it gets harder to reason about and eventually the complexity makes everyone want to start over. |
|
@Sean-Der It's a patch for blocking write based on currently read/write loop, the feature is useful for flow control in sfu forwarding reliable message and other people request it for long time. I think the change is easy to understand that the queueBuffer works as a ongoing write packet buffer in block mode and other write needs to wait for it drained. I agree the write drive will make the write more straghtforward but we might still need buffer/notify to implement non-block writing if we don't want to break the API. So it is more like a v2 feature, what do you think? |
Block write is useful for cases like sfu to forward reliable message between clients and detach the data channel to use it as normal golang connections. The default behavior is still non-block mode and the user need to enable block mode by the config. Will also add an SettingEngine option to pion/webrtc for this.
Relates to #77 and #314