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

Add an API to provide a way to get errno of juice_send #283

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mojyack
Copy link
Contributor

@mojyack mojyack commented Dec 25, 2024

The goal of this PR is to allow users to get the failure reason of juice_send.
juice_send only tells whether the transmission failed or not.
However, sometimes knowing the failure reason can help to fix it.

For example, It would be helpful to be able to do like this:

if(juice_send(...) != JUICE_ERR_SUCCESS) {
    if(errno==EAGAIN) ... // retry later
    if(errno==EMSGSIZE) ... // tell user to make packets smaller
    ...
}

Currently, conn_*_send returns -1 on failure.
Return more meaningful code to make debugging easier.
(ret < 0 && ret == SENETUNREACH) is always false
Add a new juice_send variant to expose the conn_send result to the user.
@paullouisageneau
Copy link
Owner

Thank you for the PR, the idea is very good but I don't really like the API for two reasons:

  • The return code is already there to return more specific errors, it feels clumsy to introduce a new function to write a second error code via an argument.
  • I don't want to expose platform-specific things in the API. The names EAGAIN or ESIZE are not available on every platform and it would require proper defines, with mapping to WSA errors on Windows for instance.

I think it would be cleaner to simply return new error values like JUICE_ERR_TOO_LARGE. This is retro-compatible since any new negative value should be considered an error by existing user implementations. As a side note, EAGAIN should never be returned.

@mojyack
Copy link
Contributor Author

mojyack commented Jan 8, 2025

Thank you for taking the time.
That makes sense, I'll remake patches.

As a side note, EAGAIN should never be returned.

Could you elaborate on this a little more?
I actually get EAGAIN sometimes when transferring large amounts of data.

@paullouisageneau
Copy link
Owner

Thank you for taking the time.
That makes sense, I'll remake patches.

Great, thanks!

As a side note, EAGAIN should never be returned.

Could you elaborate on this a little more? I actually get EAGAIN sometimes when transferring large amounts of data.

Oh, my bad, I thought that I changed EAGAIN/EWOULDBLOCK to return success, but I only removed the warning in that case (it happens when the buffer is full). You could introduce a JUICE_ERR_AGAIN for both of them.

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.

2 participants