Thanks for helping me figure this out @paullouisageneau. I think I've addressed most of the things discussed in the original issue. I also added a test too. There might be some things you want to change though so this PR is open to maintainer edits. Also some guesswork and copying-and-pasting was done, so there might be some dead code or things that don't make sense. And I think I have my tabs and spaces straight... although my sanity has been permanently lost
Some Things you Might Want to Review
I just made DNS resolution's defined behavior blocking in JUICE_CONCURRENCY_MODE_USER. There were some things that confused me about asynchronous DNS handling namely I think conn_impl can be NULL until DNS resolution is finished? I also just like the fact that it means there are no threads required when using this concurrency mode. I think it's reasonable to make DNS resolution the user's problem, but feel free to rewrite this part if you want non-blocking DNS resolution.
Generally the smallest way to use juice_user_poll is a little verbose. It is basically like recv(3) even to the point where juice_user_poll returns for all packets including those only used for ICE. This may have some practical use, but it's more of a side-effect of the implementation. It's also a bit disjoint since everything should still be handled in on_recv. This at least
int ret;
do {
char buffer[1500];
int ret = juice_user_poll(agent, buffer, sizeof(buffer));
} while (ret > 0);
if (ret < 0) {
// Handle error
}
The benefit of pushing the loop out of juice_user_poll here rather than calling on_recv in the function over and over is to facilitate zero-copys which I give an example of in user.c.
Each agent has its own lock like we discussed in #214 so agent use across threads should be thread safe for everything except juice_destroy if I did everything correctly
The last condition in this expression seems like it shouldn't be needed, but I needed it to make the connection establish in a reasonable amount of time. I'm thinking there is something going wrong with how the timer is set, but it's a bit beyond my understanding to fix this.
Now that I think about it juice_user_poll should probably be named juice_user_recv or juice_user_recvfrom... Maybe even give the option to pass platform flags? I'm not sure how practical that would be though
Thanks for helping me figure this out @paullouisageneau. I think I've addressed most of the things discussed in the original issue. I also added a test too. There might be some things you want to change though so this PR is open to maintainer edits. Also some guesswork and copying-and-pasting was done, so there might be some dead code or things that don't make sense. And I think I have my tabs and spaces straight... although my sanity has been permanently lost
Some Things you Might Want to Review
I just made DNS resolution's defined behavior blocking in
JUICE_CONCURRENCY_MODE_USER
. There were some things that confused me about asynchronous DNS handling namely I thinkconn_impl
can beNULL
until DNS resolution is finished? I also just like the fact that it means there are no threads required when using this concurrency mode. I think it's reasonable to make DNS resolution the user's problem, but feel free to rewrite this part if you want non-blocking DNS resolution.Generally the smallest way to use
juice_user_poll
is a little verbose. It is basically likerecv(3)
even to the point wherejuice_user_poll
returns for all packets including those only used for ICE. This may have some practical use, but it's more of a side-effect of the implementation. It's also a bit disjoint since everything should still be handled inon_recv
. This at leastThe benefit of pushing the loop out of
juice_user_poll
here rather than callingon_recv
in the function over and over is to facilitate zero-copys which I give an example of inuser.c
.Each agent has its own lock like we discussed in #214 so agent use across threads should be thread safe for everything except
juice_destroy
if I did everything correctlyThe last condition in this expression seems like it shouldn't be needed, but I needed it to make the connection establish in a reasonable amount of time. I'm thinking there is something going wrong with how the timer is set, but it's a bit beyond my understanding to fix this.