Skip to content
This repository was archived by the owner on Aug 23, 2022. It is now read-only.

Conversation

@melekes
Copy link
Contributor

@melekes melekes commented May 31, 2022

Closes #13

@k0nserv
Copy link
Member

k0nserv commented Jun 6, 2022

I think it's sensible to allow reading out all buffered data after close, but shouldn't an error still be caused in the event that you try to read after the buffered data is exhausted and the stream is closed?

Alternatively if it makes sense to close either the reading half or the writing half for long periods of time then shouldn't the state be split i.e. read_closed and write_closed?

@melekes
Copy link
Contributor Author

melekes commented Jun 6, 2022

I think it's sensible to allow reading out all buffered data after close, but shouldn't an error still be caused in the event that you try to read after the buffered data is exhausted and the stream is closed?

That's not how TCP stream behaves, neither SCTP should.

async-std TCPStream closes everything when value is dropped, but, if not, it provides shutdown method https://docs.rs/async-std/latest/async_std/net/struct.TcpStream.html#method.shutdown, which allows you to close individual portions (or both).

Interestingly enough, async-std TCPStream does not close writing side upon poll_close - https://docs.rs/async-io/1.3.1/async_io/struct.Async.html#closing. It flushes the data.

@melekes
Copy link
Contributor Author

melekes commented Jun 6, 2022

Alternatively if it makes sense to close either the reading half or the writing half for long periods of time then shouldn't the state be split i.e. read_closed and write_closed?

That makes sense. Along with adding shutdown fn https://docs.rs/async-std/latest/async_std/net/struct.TcpStream.html#method.shutdown which is going to provide more granular control over which parts are shutdown.

@melekes melekes marked this pull request as draft June 6, 2022 14:45
melekes added 3 commits June 8, 2022 11:23
- deprecate Stream::close in favor of it
- only shutdown writing half of stream in PollStream::poll_shutdown

Refs webrtc-rs#13 (comment)
@melekes melekes marked this pull request as ready for review June 8, 2022 09:03
@codecov
Copy link

codecov bot commented Jun 8, 2022

Codecov Report

Merging #14 (51ee431) into main (abd0135) will increase coverage by 0.09%.
The diff coverage is 50.87%.

❗ Current head 51ee431 differs from pull request most recent head 49a71df. Consider uploading reports for the commit 49a71df to get more accurate results

@@            Coverage Diff             @@
##             main      #14      +/-   ##
==========================================
+ Coverage   40.18%   40.27%   +0.09%     
==========================================
  Files          48       48              
  Lines        7765     7818      +53     
  Branches     2226     2249      +23     
==========================================
+ Hits         3120     3149      +29     
  Misses       3217     3217              
- Partials     1428     1452      +24     
Impacted Files Coverage Δ
examples/ping.rs 0.00% <0.00%> (ø)
examples/pong.rs 0.00% <0.00%> (ø)
src/stream/stream_test.rs 48.43% <52.72%> (+4.43%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update abd0135...49a71df. Read the comment docs.

@melekes
Copy link
Contributor Author

melekes commented Jun 8, 2022

@k0nserv ready for another review. PTAL 😸

Copy link
Member

@k0nserv k0nserv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat!

when stream's reading half is shutdown
@melekes melekes self-assigned this Jun 8, 2022
@melekes melekes merged commit 89128d7 into webrtc-rs:main Jun 14, 2022
@melekes melekes deleted the anton/13-allow-reading-after-close branch June 14, 2022 11:58
melekes added a commit to melekes/webrtc that referenced this pull request Jun 23, 2022
melekes added a commit to melekes/webrtc-data that referenced this pull request Jun 23, 2022
NOTE: we don't need to shutdown the stream as it was already done by the
association minus sending the reset request, which is not needed in this
case (i.e. if we've just received one, we don't need to send one back in
response).

Refs webrtc-rs/sctp#14
melekes added a commit to webrtc-rs/data that referenced this pull request Jun 23, 2022
* return early if the incoming stream was reset

NOTE: we don't need to shutdown the stream as it was already done by the
association minus sending the reset request, which is not needed in this
case (i.e. if we've just received one, we don't need to send one back in
response).

Refs webrtc-rs/sctp#14

* do not account for data, which we've failed to send

* format code

* only count number of bytes actually sent

not the number of bytes we're going to send
https://github.com/webrtc-rs/data/pull/11/files#r904854127
melekes added a commit to webrtc-rs/webrtc that referenced this pull request Jun 23, 2022
algesten pushed a commit to webrtc-rs/webrtc that referenced this pull request Aug 23, 2022
* return early if the incoming stream was reset

NOTE: we don't need to shutdown the stream as it was already done by the
association minus sending the reset request, which is not needed in this
case (i.e. if we've just received one, we don't need to send one back in
response).

Refs webrtc-rs/sctp#14

* do not account for data, which we've failed to send

* format code

* only count number of bytes actually sent

not the number of bytes we're going to send
https://github.com/webrtc-rs/data/pull/11/files#r904854127
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reading is not possible after closeing the Stream

2 participants