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.

Sign up for GitHub

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

fixing a couple of bugs in sctp.c in burst transfers #2427

Merged
merged 1 commit into from
Nov 23, 2020
Merged

fixing a couple of bugs in sctp.c in burst transfers #2427

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 28 additions & 17 deletions sctp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ static janus_sctp_pending_message *janus_sctp_pending_message_create(uint16_t id
if(buf != NULL && len > 0) {
m->buf = g_malloc(len);
memcpy(m->buf, buf, len);
m->len = len;
} else {
m->buf = NULL;
m->len = 0;
Expand Down Expand Up @@ -104,6 +105,7 @@ int janus_sctp_send_text_or_binary(janus_sctp_association *sctp, uint16_t id, gb
void janus_sctp_reset_outgoing_stream(janus_sctp_association *sctp, uint16_t stream);
void janus_sctp_send_outgoing_stream_reset(janus_sctp_association *sctp);
int janus_sctp_close_channel(janus_sctp_association *sctp, uint16_t id);
void janus_sctp_data_ready(janus_sctp_association *sctp);
void janus_sctp_handle_open_request_message(janus_sctp_association *sctp, janus_datachannel_open_request *req, size_t length, uint16_t stream);
void janus_sctp_handle_open_response_message(janus_sctp_association *sctp, janus_datachannel_open_response *rsp, size_t length, uint16_t stream);
void janus_sctp_handle_open_ack_message(janus_sctp_association *sctp, janus_datachannel_ack *ack, size_t length, uint16_t stream);
Expand Down Expand Up @@ -197,6 +199,7 @@ janus_sctp_association *janus_sctp_association_create(janus_dtls_srtp *dtls, jan
sctp->buffer = NULL;
sctp->buflen = 0;
sctp->offset = 0;
sctp->pending_messages = NULL;
#ifdef DEBUG_SCTP
sctp->debug_dump = NULL;
#endif
Expand Down Expand Up @@ -422,21 +425,7 @@ static int janus_sctp_incoming_data(struct socket *sock, union sctp_sockstore ad
void janus_sctp_send_data(janus_sctp_association *sctp, char *label, char *protocol, gboolean textdata, char *buf, int len) {
if(sctp == NULL)
return;
if(sctp->pending_messages != NULL && !g_queue_is_empty(sctp->pending_messages)) {
/* Messages waiting in the queue, send those first */
janus_sctp_pending_message *m = g_queue_peek_head(sctp->pending_messages);
while(m != NULL) {
int res = janus_sctp_send_text_or_binary(sctp, m->id, m->textdata, m->buf, m->len);
if(res == -2) {
JANUS_LOG(LOG_WARN, "[%"SCNu64"] Got EAGAIN when trying to resend pending message on channel %"SCNu16"\n",
sctp->handle_id, m->id);
break;
}
(void)g_queue_pop_head(sctp->pending_messages);
janus_sctp_pending_message_free(m);
m = g_queue_peek_head(sctp->pending_messages);
}
}

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure removing this is a good idea. The main reason why we tried to send all pending messages before sending the data we were just asked to send was to ensure the proper order of the messages. Since you removed this, and now pending messages are sent by the "dry" event, it means it's going to be very likely that a message sent now with janus_sctp_send_data will be sent before a pending message waiting to be sent.

Copy link
Member

Choose a reason for hiding this comment

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

No, this can not happen.
janus_sctp_send_data will either enqueue the message (in case we are buffering in pending_message) or send it right away.
Once DRY_EVENT occurs, the pending_message will be traversed in FIFO order.

if(buf == NULL || len <= 0)
return;
if(label == NULL)
Expand Down Expand Up @@ -887,6 +876,29 @@ int janus_sctp_close_channel(janus_sctp_association *sctp, uint16_t id) {
return 0;
}

void janus_sctp_data_ready(janus_sctp_association *sctp) {
if(sctp == NULL || g_atomic_int_get(&sctp->destroyed))
return;

if(sctp->pending_messages != NULL && !g_queue_is_empty(sctp->pending_messages)) {
/* Messages waiting in the queue, send those first */
janus_sctp_pending_message *m = g_queue_peek_head(sctp->pending_messages);
while(m != NULL) {
int res = janus_sctp_send_text_or_binary(sctp, m->id, m->textdata, m->buf, m->len);
if(res == -2) {
JANUS_LOG(LOG_WARN, "[%"SCNu64"] Got EAGAIN when trying to resend pending message on channel %"SCNu16"\n",
sctp->handle_id, m->id);
break;
}
(void)g_queue_pop_head(sctp->pending_messages);
janus_sctp_pending_message_free(m);
m = g_queue_peek_head(sctp->pending_messages);
}
}

janus_dtls_sctp_data_ready(sctp->dtls);
}

void janus_sctp_handle_open_request_message(janus_sctp_association *sctp, janus_datachannel_open_request *req, size_t length, uint16_t stream) {
janus_sctp_channel *channel;
uint32_t pr_value;
Expand Down Expand Up @@ -1396,8 +1408,7 @@ void janus_sctp_handle_notification(janus_sctp_association *sctp, union sctp_not
break;
case SCTP_SENDER_DRY_EVENT: {
/* Internal buffers empty, notify the application they can send again */
if(sctp != NULL && !g_atomic_int_get(&sctp->destroyed))
janus_dtls_sctp_data_ready(sctp->dtls);
janus_sctp_data_ready(sctp);
break;
}
case SCTP_NOTIFICATIONS_STOPPED_EVENT:
Expand Down