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 SocketOptionError unknown error #24

Closed
wants to merge 1 commit into from

Conversation

dragonnn
Copy link
Contributor

@dragonnn dragonnn commented Nov 1, 2024

During debugging (probably will open an issue about it soon) DtlsSocket I did hit the _ => panic!("Unknown error code: {}", errno), in SocketOptionError.
I think it is better to return an Unknown error instead of crashing the whole application.

@diondokter
Copy link
Owner

Hi, thanks for the PR!

I deliberately left it be a panic. The codes all need to be handled correctly or the socket can get into weird undefined states. The only thing the user could do is panic or close the socket and since I can't force users to close the socket (not ergonomically at least), panic is the only option I have.

And if the panic wasn't there, you might have just accepted it being there if it was easily recoverable and I wouldn't become aware of it and the crate would never handle it properly.
So the panic works, because you're here now 😉

So I'm inclined to keep the panic, because it genuinely should never panic and if it does I need to hear about it.

@dragonnn
Copy link
Contributor Author

dragonnn commented Nov 1, 2024

Hmmm I would be probably here too if the DtlsConnect returned an error :D. But I am fine with keeping it as I already found the issue and it clearly my error: #25

@diondokter
Copy link
Owner

Hmmm I would be probably here too if the DtlsConnect returned an error :D. But I am fine with keeping it as I already found the issue and it clearly my error: #25

Ha good, but sadly not everyone is such a good Samaritan :P

@diondokter diondokter closed this Nov 1, 2024
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