Getting access to lua thread objects about cqueues HOT 10 CLOSED

I had a go at adding this feature tonight. over on a branch: https://github.com/daurnimator/cqueues/commits/thread-on-error
Please review :)

from cqueues.

On Sun, Jan 11, 2015 at 08:34:26PM -0800, daurnimator wrote:

I had a go at adding this feature tonight. over on a branch: https://github.com/daurnimator/cqueues/commits/thread-on-error
Please review :)

One of the TODO items I had in mind was to tweak cqueues:wrap and
cqueues:attach to return a unique identifier which could then be used to
communicate information about a context just like this.

The actual thread object is an obvious choice, but what if we want to
eventually recycle threads? If we provide context for errors, it would make
sense to also provide a way to communicate which threads finished normally.
If we use the thread object as the identifier returned from cqueues:attach
and :wrap, then we can't reycle the threads--maybe somebody put it in a weak
table so it could detect when it's GC'd.

But maybe that's not a particularly useful feature. It also conflicts with
the return signature and usage pattern of :step, and perhaps other
inconsistencies.

Your patch still works, but if something like the above was implement we
might need to return 4 values.

from cqueues.

The actual thread object is an obvious choice, but what if we want to
eventually recycle threads?

Well that doesn't work with :attach anyway.

Reusing threads with :wrap is interesting though.

If we provide context for errors, it would make
sense to also provide a way to communicate which threads finished normally.
If we use the thread object as the identifier returned from cqueues:attach
and :wrap, then we can't reycle the threads--maybe somebody put it in a weak
table so it could detect when it's GC'd.

Does it?

You cannot recycle a thread that has had an error, it is "dead" (as returned by coroutine.status).
I limited the patch to this case (only errors), as there was nothing else you can use the thread object for again.

The actual 'thread' object is required, as I will want to be able to use it with the debug.* apis that take a thread as a first parameter.

But maybe that's not a particularly useful feature. It also conflicts with
the return signature and usage pattern of :step, and perhaps other
inconsistencies.

Your patch still works, but if something like the above was implement we
might need to return 4 values.

I think returning the thread in the :step() failure case is absolutely fine.
What to return in the success/running case can be left as an open question.

from cqueues.

Any more thoughts here?

from cqueues.

I'm sold. Only question mark is what the return signature should be for :step. Should it be

local ok, errmsg, errno, thr = loop:step()

or

local ok, errmsg, errno_or_thr = loop:step()

or something else? Some errors might not be related to a specific thread. Or a thread error might have an associated errno.

from cqueues.

Hmm, I didn't think about errno. at least in the handler I modified in my branch I didn't see it...

Or a thread error might have an associated errno.

How would that occur?

from cqueues.

Infact, are there any circumstances where :step returns an errno?

from cqueues.

Right now, no. But that's because we either throw on error, or discard the errno after formatting the error message. Even if we return the thread, some of those thread errors involve a system error that we would want to return directly so the user doesn't have to parse the message.

I'm thinking maybe the return signature should look like:

local ok, msg, errno, thr = loop:step()

or even

local ok, msg, errno, thr, obj = loop:step()

were obj is the object that triggered the error.

The most common error will likely be en error thrown by the thread itself, in which case errno will be nil and it might seem dumb to have a hole in the list like that. But it seems the best out of all the alternatives. Thoughts?

from cqueues.

Right now, no.

So lets add it if we think it's reasonable. Should be lua_pushinteger(L, error) at line 1671: https://github.com/wahern/cqueues/blob/master/src/cqueues.c#L1671 and then an extra thing to xmove

I'm thinking maybe the return signature should look like:

local ok, msg, errno, thr = loop:step()
or even

local ok, msg, errno, thr, obj = loop:step()
were obj is the object that triggered the error.

Lets go with the former for now. we can add the poll object later if it seems to be required.

from cqueues.

Fixed with 0c43868, followed by bug fix in 8f6645f.

from cqueues.