Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update datachannel and sctp to include SetReadDeadline #2369

Merged
merged 1 commit into from Dec 12, 2022

Conversation

ckousik
Copy link
Contributor

@ckousik ckousik commented Dec 12, 2022

Description

Datachannel and SCTP recently merged changes which allow setting read deadlines. This PR updates the datachannel and SCTP versions until tagged releases can be made for both repositories.

Reference issue

Fixes #...

@ckousik
Copy link
Contributor Author

ckousik commented Dec 12, 2022

@edaniels could you take a look at this too?

@edaniels
Copy link
Member

@ckousik on it. just pushed on your branch to get true tagged versions so we can get your PR in

@ckousik
Copy link
Contributor Author

ckousik commented Dec 12, 2022

Thank you!

@edaniels edaniels changed the title Update datachannel and sctp Update datachannel and sctp to include DataChannel/SCTP SetReadDeadline Dec 12, 2022
@edaniels edaniels changed the title Update datachannel and sctp to include DataChannel/SCTP SetReadDeadline Update datachannel and sctp to include SetReadDeadline Dec 12, 2022
@edaniels
Copy link
Member

@ckousik I messed up on pion/datachannel@6d03d00. It breaks backwards compatibility by adding a new interface method. I'm going to extract it out into a new interface but in a future major release, it can be incorporated back in.

@edaniels
Copy link
Member

See pion/datachannel@90f6b24

@codecov
Copy link

codecov bot commented Dec 12, 2022

Codecov Report

Base: 77.46% // Head: 70.15% // Decreases project coverage by -7.31% ⚠️

Coverage data is based on head (1c87561) compared to base (720d9b0).
Patch has no changes to coverable lines.

❗ Current head 1c87561 differs from pull request most recent head 10fdfce. Consider uploading reports for the commit 10fdfce to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2369      +/-   ##
==========================================
- Coverage   77.46%   70.15%   -7.32%     
==========================================
  Files          87       64      -23     
  Lines        9275     3354    -5921     
==========================================
- Hits         7185     2353    -4832     
+ Misses       1655      873     -782     
+ Partials      435      128     -307     
Flag Coverage Δ
go ?
wasm 70.15% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
stats.go 0.00% <0.00%> (-100.00%) ⬇️
constants.go 0.00% <0.00%> (-100.00%) ⬇️
sdpsemantics.go 0.00% <0.00%> (-82.76%) ⬇️
icecandidatepair.go 0.00% <0.00%> (-81.82%) ⬇️
rtpcodec.go 13.33% <0.00%> (-80.00%) ⬇️
track_local.go 0.00% <0.00%> (-66.67%) ⬇️
icemux.go 0.00% <0.00%> (-54.55%) ⬇️
atomicbool.go 57.14% <0.00%> (-42.86%) ⬇️
internal/mux/endpoint.go 13.79% <0.00%> (-37.94%) ⬇️
dtlsrole.go 71.42% <0.00%> (-28.58%) ⬇️
... and 37 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@edaniels
Copy link
Member

Oof, you're bumping up against a regression in SCTP I believe. Let me see if I can work it out real quick. Otherwise, this will need to wait a bit

@edaniels
Copy link
Member

Potentially blocked by pion/sctp#254

@edaniels
Copy link
Member

Ended up reverting the failing commit in sctp to get a new version out (and put it back into master after). pion/sctp#255 tracks the issue. Will put this in and bump shortly.

To include SetReadDeadline. A ReadDeadliner can now
be used from an underlying DataChannel
@edaniels edaniels merged commit 1c20cd4 into pion:master Dec 12, 2022
@edaniels
Copy link
Member

Merged and bumped! I'm not sure how you plan on using the new method though? Will you use detach?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants