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

Update modules to be Lua coroutine compatible #1167

Closed
wants to merge 1 commit into from

Conversation

TerryE
Copy link
Collaborator

@TerryE TerryE commented Mar 17, 2016

Changes as per #846. I still have to do fill testing. I can do the net, uart and wifi ones. COAP is pretty moribund anyway at the moment. This is more to provoke feedback comment than to merge just yet.

@@ -121,12 +121,12 @@ static void net_socket_disconnected(void *arg) // tcp only
if(nud->pesp_conn)
c_free(nud->pesp_conn);
nud->pesp_conn = NULL; // espconn is already disconnected
lua_gc(gL, LUA_GCSTOP, 0);
lua_gc(L, LUA_GCSTOP, 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a matter of interest, why is the GC disabled here for a short time? I don't think that it needs to be..... [Worse, if it does need to be disabled, then the disable call is probably missing from lots of other places as well]

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't know. I could take a look but I think that I'll leave this until we fix the net module properly.

@TerryE
Copy link
Collaborator Author

TerryE commented Mar 29, 2016

These seem to be stable. Anyone found any probs?

@dnc40085
Copy link
Contributor

dnc40085 commented Apr 7, 2016

@TerryE I'm pretty sure that the conflicts are caused by #1018, which includes the lua coroutine compatibility fix for wifi.c.

@TerryE
Copy link
Collaborator Author

TerryE commented Apr 8, 2016

@dnc40085 @jmattsson @devsaurus @pjsg, you are the guys who have worked on these stacks in the past. This PR has been sitting here for 3 weeks. I am assuming that no one has any issues other than the merge conflicts created by later PRs that have been accepted as dnc40085 mentions. If I don't hear to the contrary in the next day or so, then I'll remove these conflicts and merge this PR myself.

@TerryE
Copy link
Collaborator Author

TerryE commented Apr 8, 2016

As a matter of interest, why is the GC disabled here [in net_socket_disconnect] for a short time? I don't think that it needs to be.

@pjsg Philip, it's a cludge, and it's used in a couple of other places in the module. The net.socket userdata include a self-reference via the registry, and this prevents GC of the userdata even if the socket isn't in scope anywhere in Lua. This can happen in circumstances like:

do
  local svr = net.createServer(net.TCP)
  srv:listen(888*,accept_cb)
end

Here without this reference, the GC could clean up the socket after exiting the do .. end and so it would have been freed by the time the accept_cb() fires. Unreferencing an entry in the Lua Registry can trigger an ECG run which will reclaim the userdata before the code is finished with it, hence this cludge. The correct Lua approach is to "turn out the lights when you leave the room", i.e. do the unregister last when the function doesn't need the userdata any more.

@jmattsson
Copy link
Member

Happy to see this merged after conflicts resolved.

@TerryE
Copy link
Collaborator Author

TerryE commented May 9, 2016

Sorry guys, I am now on a Greek Island and my only access is by logging on from my local Taverna.

I have also been fire-fighting an issue with a forum where I am an active contributor and it's gone belly-up because the sole-owner decided to take his ball and go home (shut it down). With most of the mods and senior contributors, I am trying to do a phoenix -- the only prob is that I am the only experienced Sysdmin / PHP programmer / MySQL admin so it is sucking up all of my spare time for next week or so.

Juggling priorities and still trying to have a holiday.

@djphoenix
Copy link
Contributor

Should I recreate this PR with resolved conflicts? Have merged to my repo successfully, all seems OK.

@pjsg
Copy link
Member

pjsg commented Jul 21, 2016

@djphoenix -- yes, please restage and we can get this squared away....

@TerryE TerryE deleted the dev-couroutine-fixes branch August 21, 2020 17:30
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.

6 participants