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

Fix undefined behaviour #24

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

Conversation

daurnimator
Copy link

@daurnimator daurnimator commented Feb 1, 2018

Closes #20 and #23

@daurnimator daurnimator changed the title Save current lua_State for callbacks to use Fix undefined behaviour Feb 1, 2018
@daurnimator
Copy link
Author

Ping?

@karlp
Copy link
Contributor

karlp commented Mar 5, 2018

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.

@ironpillow
Copy link

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
@lePereT
Copy link

lePereT commented May 13, 2021

Please could this be merged. Will allow this library to be used in concurrent programs making use of co-routines

@niecore
Copy link

niecore commented Jun 11, 2021

Any updates on this PR?

@karlp
Copy link
Contributor

karlp commented Jun 11, 2021

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.

@icarus75
Copy link
Member

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:
This commit changes errors from undefined behaviour (that luckily used to get reported) to silently discarding errors.
Having a solid set of unit tests with good code coverage would help. Any takers?

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.

Callbacks occur in same coroutine that mqtt.new is called from
6 participants