-
Notifications
You must be signed in to change notification settings - Fork 42
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
Fix undefined behaviour #24
base: master
Are you sure you want to change the base?
Conversation
Ping? |
not forgotten. Just been really busy at work, and because I don't understand lua as well as you, I need to run this through my own tests and try this out before I'm happy with it. |
Have similar issues. @karlp can you merge this PR. Really appreciate it! |
Lua errors (including allocation failures) would `longjmp` out of the callbacks, bypassing cleanup in libmosquitto. This commit changes errors from undefined behaviour (that luckily used to get reported) to silently discarding errors. Closes flukso#23
Please could this be merged. Will allow this library to be used in concurrent programs making use of co-routines |
Any updates on this PR? |
For my sake at least, I simply felt uncomfortable with how much this PR had to change. I needed to do a lot more testing on it to understand it, and it has simply never been a priority for me. I've still got this ticket as an "unread email" because it really does appear to be a real issue, but I'm personally unlikely to ever find the time to wrap my head around it. (I'm not a co-routine user, really at all) One of the other maintainers may have more time allocated for this. My complete lack of co-routine usage just make this a hard project. |
I second @karlp. I also don't have the time to really get to the bottom of this. My major concern is that these proposed changes would cause a regression in existing functionality, which seems warranted with commit statements like: |
Closes #20 and #23