-
Notifications
You must be signed in to change notification settings - Fork 41
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
Implement datachannel close #24
base: master
Are you sure you want to change the base?
Conversation
Sets SCTP_RESET_STREAMS on the socket to close the datachannel.
cf74be8
to
6f98e0e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, thank you so much. A few things I'd like cleaned up before its time to merge.
I'll try to take some time this evening to more thoroughly review and test out this PR.
src/SCTPWrapper.cpp
Outdated
@@ -98,6 +98,42 @@ void SCTPWrapper::OnNotification(union sctp_notification *notify, size_t len) { | |||
break; | |||
case SCTP_STREAM_RESET_EVENT: | |||
SPDLOG_TRACE(logger, "OnNotification(type=SCTP_STREAM_RESET_EVENT)"); | |||
struct sctp_stream_reset_event reset_event; | |||
reset_event = notify->sn_strreset_event; | |||
for (int i = 1; i < 2; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets drop the for
here - we're only closing one data channel at a time.
Alternatively, if the sctp_stream_reset_event
can actually hold multiple streams to reset, lets loop across all of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The event could hold multiple streams as it's a list. I would ideally want to loop across each of them, but I'm not sure how to do that without knowing the size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix in 289b7ef
include/rtcdcpp/PeerConnection.hpp
Outdated
void HandleStringMessage(ChunkPtr chunk, uint16_t sid); | ||
void HandleBinaryMessage(ChunkPtr chunk, uint16_t sid); | ||
|
||
std::shared_ptr<Logger> logger = GetLogger("rtcdcpp.PeerConnection"); | ||
public: | ||
void ResetSCTPStream(uint16_t stream_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to hide the specifics of resetting streams inside the DataChannel - the PeerConnection here is meant to be a somewhat close mirror of the Javascript DataChannel API, which doesn't expose SCTP directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree! I shouldn't expose this. Will fix soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 91db2d4. Let me know if this is okay.
src/SCTPWrapper.cpp
Outdated
logger->info("Outgoing stream_id#{} have been reset, calling onClose CB", streamid); | ||
const uint8_t dc_close_data = DC_TYPE_CLOSE; | ||
const uint8_t *dc_close_ptr = &dc_close_data; | ||
OnMsgReceived(dc_close_ptr, sizeof(dc_close_ptr), streamid + 1, PPID_CONTROL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this streamid + 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 55b64e1
Also fixes the function prototype.
This is computed from strreset_length field from sctp_stream_reset_event struct. This field is the total length in bytes of the delivered event, including the header. https://tools.ietf.org/html/rfc6525#section-6.1.1
* Heap allocation of struct and free it soon after * Simplified 'len'
No description provided.