LuaTask needs your help

classic Classic list List threaded Threaded
21 messages Options
12
Reply | Threaded
Open this post in threaded view
|

LuaTask needs your help

Daniel Quintela

I'm almost ready to release LuaTask 1.6.2

I changed the way TASK_ENTRY memory is allocated.

I would appreciate it very much if you could test it and give me some feedback.

You can get the candidate from: http://www.soongsoft.com/lua/ltask.c


Many thanks,
Daniel



Reply | Threaded
Open this post in threaded view
|

Re: LuaTask needs your help

Duck-2

It seems to work across TASK_SLOTS_STEP boundaries when I use the test
code I had before. I tried creating 10 simultanous tasks, then 20, 40, 80,
160, 320 -- all good!

However, I can't create more than 509 tasks, i.e. 510 in total, including
the "_main_".

When I call task.create() for the 510th time, I get error -3 (can't create message queue). This happens with PTHREAD_STACK_SIZE set to 1m or to 2m. With the default PTHREAD_STACK_SIZE of 8m I get error -4 (can't create os thread) at the 383rd task, presumably because I have exhausted all 3g of virtual memory at that point.

Note that this isn't a new problem -- it exists with the 1.6.1 code as
well. I didn't realise it before because the segfault after
(TASK_SLOT_STEP-1) calls to task.create() got in the way :-) I just went
back to 1.6.1 and compiled with TASK_SLOT_STEP set to 600 in order to see
what happens: you get err -3 from the 510th call to task.create().

About the code itself, I have two suggestions.

Firstly, when you allocate space for the TASK_ENTRYs themselves (not the
list of pointers to TASK_ENTRYs), you don't check the return value of
malloc() for NULL.

Secondly, I'd strongly suggest factoring the actual malloc/realloc code
out into a function so it appears only once. At the moment you are
repeating code (except for a malloc() versus a realloc() -- and realloc()
behaves like malloc() when you feed it NULL). This means there are two
places to introduce errors or discrepancies.

My 2c.


Reply | Threaded
Open this post in threaded view
|

Re: LuaTask needs your help

Thomas Lauer-3
In reply to this post by Daniel Quintela
Daniel Quintela <[hidden email]> wrote:
> 
> I'm almost ready to release LuaTask 1.6.2
> 
> I changed the way TASK_ENTRY memory is allocated.
> 
> I would appreciate it very much if you could test it and give me some 
> feedback.

This is too complicated, methinks. The old, simpler allocation scheme
was perfectly okay. The crashes resulted from invalid accesses, I think.
And I am not sure that these changes will solve all problems with that.

Following Kacper's clever analysis, I went through the ltask code step
by step. I finally changed just a few lines in taskthread(). This
produced a version that seems to run okay on my box. (I say "seems"
because the crashes I got were not strictly reproducible. I did
stress-test the code for a good while but with this sort of bug there's
always a small possibility that it's buggy but just refuses to crash.)

I didn't post these changes because I did other, unrelated changes as
well, so I couldn't just post a straightforward patch file. Also, I knew
you were already working on a fix anyway.

FWIW, the gist of my changes is the following. In the 1.6.1 version of
ltask.c, line 540, you initialise te:

te = aTask + ( ( long) vp);

The mutex is given up in line 554. After this line, the block aTask
points to can move at any time because of an realloc(). And if aTask
moves, then te is pointing nowhere.

However, in lines 558, 560, 567, 568, 571 ,573 and 583 te is still used.
(Getting the mutex back in 577 doesn't reinitialise te.)

I assume this is what produced the crashes I saw. When I changed the
code such that te is either not needed after line 554 or reinitialised
before accessing thread information everything seems to work fine (with
the caveat given).

Here is my current taskthread(). As I said, there are a few other
changes and I also compile this as C++ code. The three main changes are
commented:

static OS_THREAD_FUNC taskthread(void *vp) {
  TASK_ENTRY *te;
  int status=0;
  const char *init=getenv("LUA_INIT");
  OsSetThreadData(threadDataKey,vp);
  OsLockMutex(tlMutex,INFINITE);
  te=aTask+(long)vp;
#ifndef _WIN32
  pthread_setcanceltype( PTHREAD_CANCEL_ASYNCHRONOUS, NULL);
  pthread_cleanup_push( taskCleanup, te);
#endif
  lua_State *L=te->L;        // <-- save te->L before giving up mutex
  const char *fn=te->fname;  // <-- save te->fname as well
  lua_gc(L,LUA_GCSTOP,0);
  luaL_openlibs(L);
  initLTask(L);
  lua_gc(L,LUA_GCRESTART, 0);
  OsUnlockMutex(tlMutex);
  if (init!=NULL) {
    if (init[0]=='@') status=dofile(L,init+1);
    else status=dostring(L,init,"=LUA_INIT");
  }
  if (status==0) {
    if (fn[0]=='@') dofile(L,fn+1);
    else dostring(L,fn,"=STRING_TASK");
  }
  OsLockMutex(tlMutex,INFINITE);
  te=aTask+(long)vp;  // <-- reinitialise te
#ifndef _WIN32
  pthread_cleanup_pop(0);
#endif
  taskCleanup(te);
  OsUnlockMutex(tlMutex);
  return 0;
}

Sorry, probably should have posted this earlier. Anyway, HTH.

-- 
cheers  thomasl

web : http://thomaslauer.com/start


Reply | Threaded
Open this post in threaded view
|

Re: LuaTask needs your help

Kacper Wysocki
In reply to this post by Daniel Quintela
On 7/7/07, Daniel Quintela <[hidden email]> wrote:

I'm almost ready to release LuaTask 1.6.2

I changed the way TASK_ENTRY memory is allocated.

I would appreciate it very much if you could test it and give me some
feedback.

You can get the candidate from: http://www.soongsoft.com/lua/ltask.c

I'd take a look but I can't reach this server...

-K

Reply | Threaded
Open this post in threaded view
|

RES: LuaTask needs your help

Rafael - SosCpdTerra
Think the end justify the way.

-----Mensagem original-----
De: [hidden email]
[[hidden email]] Em nome de Kacper Wysocki
Enviada em: domingo, 8 de julho de 2007 08:41
Para: Lua list
Assunto: Re: LuaTask needs your help

On 7/7/07, Daniel Quintela <[hidden email]> wrote:
>
> I'm almost ready to release LuaTask 1.6.2
>
> I changed the way TASK_ENTRY memory is allocated.
>
> I would appreciate it very much if you could test it and give me some
> feedback.
>
> You can get the candidate from: http://www.soongsoft.com/lua/ltask.c

I'd take a look but I can't reach this server...

-K
/* ** $Id: $ ** "Multitasking" support ( see README ) ** SoongSoft, Argentina ** 
http://www.soongsoft.com [hidden email] ** Copyright (C) 2003-2007 
Daniel Quintela. All rights reserved. */ #include #include #include "lualib.h" 
#ifdef _WIN32 # include # include # include #ifndef NATV_WIN32 #include #endif 
#else # define _strdup strdup # define _strnicmp strncasecmp # include # include 
# include # include # include # include # include #endif #define LT_NAMESPACE 
"task" #include #include #include "syncos.h" #include "queue.h" #ifdef _WIN32 
static long ( __stdcall *LRT_LIB_OVERRIDE)( lua_State *L) = NULL; static long ( 
__stdcall *LRT_DOFILE_OVERRIDE)( lua_State *L, const char *filename) = NULL; 
#else long ( *LRT_LIB_OVERRIDE)( lua_State *L) = NULL; long ( 
*LRT_DOFILE_OVERRIDE)( lua_State *L, const char *filename) = NULL; #endif 
#define TASK_SLOTS_STEP 16 typedef struct S_TASK_ENTRY { QUEUE queue; char 
*fname; lua_State *L; OS_THREAD_T th; long running; char *id; long msgcount; } 
TASK_ENTRY; typedef struct S_MSG_ENTRY { long len; long flags; char data[1]; } 
MSG_ENTRY; typedef struct S_LOPN_LIB { char *name; unsigned long value; int 
(*fnc)(lua_State *); } LOPN_LIB; static void * tlMutex = NULL; static TASK_ENTRY 
* * volatile aTask; static long countTask = 0; static long threadDataKey; /* 
Internal functions */ static OS_THREAD_FUNC taskthread( void *vp); static int 
traceback (lua_State *L) { lua_getfield(L, LUA_GLOBALSINDEX, "debug"); if 
(!lua_istable(L, -1)) { lua_pop(L, 1); return 1; } lua_getfield(L, -1, 
"traceback"); if (!lua_isfunction(L, -1)) { lua_pop(L, 2); return 1; } 
lua_pushvalue(L, 1); /* pass error message */ lua_pushinteger(L, 2); /* skip 
this function and traceback */ lua_call(L, 2, 1); /* call debug.traceback */ 
return 1; } static int docall (lua_State *L, int narg, int clear) { int status; 
int base = lua_gettop(L) - narg; /* function index */ lua_pushcfunction(L, 
traceback); /* push traceback function */ lua_insert(L, base); /* put it under 
chunk and args */ status = lua_pcall(L, narg, (clear ? 0 : LUA_MULTRET), base); 
lua_remove(L, base); /* remove traceback function */ /* force a complete garbage 
collection in case of errors */ if (status != 0) lua_gc(L, LUA_GCCOLLECT, 0); 
return status; } static int dofile (lua_State *L, const char *name) { int status 
= luaL_loadfile(L, name) || docall(L, 0, 1); return status; } static int 
dostring (lua_State *L, const char *s, const char *name) { int status = 
luaL_loadbuffer(L, s, strlen(s), name) || docall(L, 0, 1); return status; } 
static void taskCleanup( void *vp) { TASK_ENTRY *te = ( TASK_ENTRY *) vp; 
lua_close( te->L); te->running = 0; te->L = NULL; free( te->fname); if( te->id 
!= NULL) free( te->id); QueDestroy( &( te->queue)); } static long 
int_taskcreate( const char *fname, lua_State *TL) { if( tlMutex != NULL) { long 
i; OsLockMutex( tlMutex, INFINITE); for( i = 0; i < countTask; i++) if( !( 
aTask[i]->running)) break; if( i == countTask) { TASK_ENTRY **te; long j; te = ( 
TASK_ENTRY * *) realloc( aTask, sizeof( TASK_ENTRY *) * ( countTask + 
TASK_SLOTS_STEP)); if( te == NULL) { OsUnlockMutex( tlMutex); return( -1); } 
aTask = te; aTask[i] = ( TASK_ENTRY *) malloc( sizeof( TASK_ENTRY) * ( 
TASK_SLOTS_STEP)); countTask += TASK_SLOTS_STEP; for( j = i; j < countTask; j++) 
{ aTask[j] = ( TASK_ENTRY *) ( ( ( char * ) aTask[i] ) + ( sizeof( TASK_ENTRY) * 
( j - i ) ) ); aTask[j]->running = 0; #ifdef NATV_WIN32 aTask[j]->th = 0; #endif 
} } #ifdef NATV_WIN32 if( aTask[i]->th) CloseHandle( ( HANDLE) aTask[i]->th); 
#endif if( ( aTask[i]->fname = _strdup( fname)) == NULL) { OsUnlockMutex( 
tlMutex); return( -2); } else { if( QueCreate( &( aTask[i]->queue), 
QUE_NO_LIMIT)) { free( aTask[i]->fname); OsUnlockMutex( tlMutex); return( -3); } 
} aTask[i]->id = NULL; aTask[i]->msgcount = 0; aTask[i]->L = TL; if( 
OsCreateThread( &( aTask[i]->th), taskthread, ( void *) i)) { free( 
aTask[i]->fname); QueDestroy( &( aTask[i]->queue)); OsUnlockMutex( tlMutex); 
return( -4); } aTask[i]->running = 1; OsUnlockMutex( tlMutex); return( i + 1); } 
return( -11); } static long int_taskregister( const char *id) { long lrc = -1; 
long i = ( long) OsGetThreadData( threadDataKey); if( ( i > -1) && ( i < 
countTask)) { OsLockMutex( tlMutex, INFINITE); if( aTask[i]->id != NULL) free( 
aTask[i]->id); aTask[i]->id = _strdup( id); lrc = 0; OsUnlockMutex( tlMutex); } 
return( lrc); } static void int_tasklibinit(lua_State *L) { if( tlMutex == NULL) 
{ threadDataKey = OsCreateThreadDataKey(); OsSetThreadData( threadDataKey, ( 
void *) 0); aTask = ( TASK_ENTRY * *) malloc( sizeof( TASK_ENTRY *) * 
TASK_SLOTS_STEP); if( aTask != NULL) { long i; aTask[0] = ( TASK_ENTRY *) 
malloc( sizeof( TASK_ENTRY) * TASK_SLOTS_STEP); countTask = TASK_SLOTS_STEP; 
for( i = 0; i < TASK_SLOTS_STEP; i++) { aTask[i] = ( TASK_ENTRY *) ( ( ( char * 
) aTask[0] ) + ( sizeof( TASK_ENTRY) * i ) ); aTask[i]->running = 0; #ifdef 
NATV_WIN32 aTask[i]->th = 0; #endif } tlMutex = OsCreateMutex( NULL); QueCreate( 
&( aTask[0]->queue), QUE_NO_LIMIT); aTask[0]->L = L; aTask[0]->running = 1; 
aTask[0]->id = NULL; aTask[0]->fname = "_main_"; aTask[0]->msgcount = 0; } 
#ifdef _WIN32 LRT_LIB_OVERRIDE = ( long ( __stdcall *)( lua_State *L)) 
GetProcAddress( GetModuleHandle( NULL), "_LRT_LIB_OVERRIDE@4"); 
LRT_DOFILE_OVERRIDE = ( long ( __stdcall *)( lua_State *L, const char 
*filename)) GetProcAddress( GetModuleHandle( NULL), "_LRT_DOFILE_OVERRIDE@8"); 
#endif } } /* Registered functions */ static int reg_taskcreate(lua_State *L) { 
long lrc; long ti = 0; const char *fname = luaL_checkstring(L, 1); lua_State *TL 
= lua_open(); lua_newtable(TL); lua_pushnumber(TL, 0); lua_pushstring(TL, 
fname); lua_settable( TL, -3); if( lua_istable(L, 2)) { lua_pushnil(L); while( 
lua_next(L, 2) != 0) { if( lua_isnumber(L, -1)) { lua_pushnumber( TL, ++ti); 
lua_pushnumber( TL, lua_tonumber( L, -1)); lua_settable( TL, -3); } else if( 
lua_isstring(L, -1)) { lua_pushnumber( TL, ++ti); lua_pushstring( TL, 
lua_tostring( L, -1)); lua_settable( TL, -3); } else { lua_pop(L, 1); break; } 
lua_pop(L, 1); } } lua_setglobal(TL, "arg"); lrc = int_taskcreate( fname, TL); 
if( lrc < 0) lua_close( TL); lua_pushnumber( L, lrc); return 1; } static int 
reg_taskregister( lua_State *L) { long lrc; const char *id = luaL_checkstring(L, 
1); lrc = int_taskregister( id); lua_pushnumber( L, lrc); return( 1); } static 
int reg_taskfind( lua_State *L) { long i; long lrc = -1; const char *id = 
luaL_checkstring(L, 1); OsLockMutex( tlMutex, INFINITE); for( i = 0; i < 
countTask; i++) if( aTask[i]->id != NULL) if( !strcmp( aTask[i]->id, id)) { lrc 
= i + 1; break; } OsUnlockMutex( tlMutex); lua_pushnumber( L, lrc); return( 1); 
} static int reg_taskunregister(lua_State *L) { long lrc; lrc = 
int_taskregister( ""); lua_pushnumber( L, lrc); return( 1); } static int 
reg_taskpost(lua_State *L) { const char *buffer; MSG_ENTRY *me; size_t len; long 
idx = ( long) luaL_checknumber(L, 1); long flags = ( long) luaL_optinteger(L, 3, 
0); long lrc = -1; buffer = luaL_checklstring(L, 2, &len); idx--; OsLockMutex( 
tlMutex, INFINITE); if( ( idx > -1) && ( idx < countTask)) { me = malloc( 
sizeof( MSG_ENTRY) + len - 1); if( me == NULL) { lrc = -2; } else { me->len = 
len; me->flags = flags; memcpy( me->data, buffer, len); aTask[idx]->msgcount++; 
QuePut( &( aTask[idx]->queue), me); lrc = 0; } } OsUnlockMutex( tlMutex); 
lua_pushnumber( L, lrc); return 1; } static int reg_tasklist(lua_State *L) { 
long i; lua_newtable( L); for( i = 0; i < countTask; i++) if( aTask[i]->running) 
{ lua_pushnumber( L, i + 1); lua_newtable( L); lua_pushstring( L, "script"); if( 
aTask[i]->fname[0] == '=' ) lua_pushstring( L, "STRING_TASK"); else 
lua_pushstring( L, aTask[i]->fname); lua_settable( L, -3); lua_pushstring( L, 
"msgcount"); lua_pushnumber( L, aTask[i]->msgcount); lua_settable( L, -3); if( 
aTask[i]->id != NULL) { lua_pushstring( L, "id"); lua_pushstring( L, 
aTask[i]->id); lua_settable( L, -3); } lua_settable( L, -3); } return 1; } 
static int reg_taskreceive(lua_State *L) { MSG_ENTRY *me; long lrc = -1; long i 
= ( long) OsGetThreadData( threadDataKey); long tout = ( long) 
luaL_optinteger(L, 1, INFINITE); if( i > -1) { #ifdef _WIN32 HANDLE qwh = ( 
HANDLE) GetQueNotEmptyHandle( &( aTask[i]->queue)); DWORD dwWait = 
WaitForSingleObjectEx( qwh, tout, TRUE); if( ( aTask[i]->running == 1) && ( 
dwWait == WAIT_OBJECT_0)) { #else int psw; struct pollfd pfd; pfd.fd = 
GetQueNotEmptyHandle( &( aTask[i]->queue)); pfd.events = POLLIN; psw = poll( 
&pfd, 1, tout); if( ( aTask[i]->running == 1) && ( psw == 1)) { #endif _QueGet( 
&( aTask[i]->queue), ( void **) &me); aTask[i]->msgcount--; lua_pushlstring( L, 
me->data, me->len); lua_pushnumber( L, me->flags); free( me); lrc = 0; } else { 
lua_pushnil( L); lua_pushnil( L); lrc = -2; } } else { lua_pushnil( L); 
lua_pushnil( L); } lua_pushnumber( L, lrc); return 3; } static int reg_taskid( 
lua_State *L) { long i = ( long) OsGetThreadData( threadDataKey); 
lua_pushnumber( L, i + 1); return( 1); } static int reg_getqhandle( lua_State 
*L) { long qwh = 0; long i = ( long) OsGetThreadData( threadDataKey); if( i > 
-1) { qwh = GetQueNotEmptyHandle( &( aTask[i]->queue)); } lua_pushnumber( L, ( 
long) qwh); return( 1); } static int reg_cancel( lua_State *L) { long running; 
long lrc = -1; long i = ( long) luaL_checknumber(L, 1); OsLockMutex( tlMutex, 
INFINITE); if( ( i > 1) && ( i <= countTask)) if( --i != ( long) 
OsGetThreadData( threadDataKey)) { lrc = 0; running = aTask[i]->running; if( 
running) { aTask[i]->running = 2; lrc = OsCancelThread( aTask[i]->th); if( 
aTask[i]->running == 2) aTask[i]->running = 0; } } OsUnlockMutex( tlMutex); 
lua_pushnumber( L, lrc); return( 1); } static int reg_isrunning( lua_State *L) { 
long running = 0; long i = ( long) luaL_checknumber(L, 1); OsLockMutex( tlMutex, 
INFINITE); if( ( i > 1) && ( i <= countTask)) running = aTask[--i]->running ? 1 
: 0; OsUnlockMutex( tlMutex); lua_pushboolean( L, running); return( 1); } /* 
Module exported function */ static const struct luaL_reg lt_lib[] = { { 
"create", reg_taskcreate}, { "register", reg_taskregister}, { "find", 
reg_taskfind}, { "receive", reg_taskreceive}, { "post", reg_taskpost}, { 
"unregister", reg_taskunregister}, { "list", reg_tasklist}, { "id", reg_taskid}, 
{ "getqhandle", reg_getqhandle}, { "cancel", reg_cancel}, { "isrunning", 
reg_isrunning}, { NULL, NULL} }; LUATASK_API int luaopen_task(lua_State *L) { 
int_tasklibinit(L); luaL_openlib (L, LT_NAMESPACE, lt_lib, 0); lua_pop (L, 1); 
return 0; } static OS_THREAD_FUNC taskthread( void *vp) { TASK_ENTRY *te; int 
status = 0; #if (_MSC_VER >= 1400) size_t l_init; char *init; 
_dupenv_s(&init,&l_init,"LUA_INIT"); #else const char *init = 
getenv("LUA_INIT"); #endif OsSetThreadData( threadDataKey, vp); OsLockMutex( 
tlMutex, INFINITE); //te = aTask + ( ( long) vp); te = aTask[(long) vp]; #ifndef 
NATV_WIN32 pthread_setcanceltype( PTHREAD_CANCEL_ASYNCHRONOUS, NULL); 
pthread_cleanup_push( taskCleanup, te); #endif lua_gc(te->L, LUA_GCSTOP, 0); /* 
stop collector during initialization */ luaL_openlibs(te->L); /* open libraries 
*/ luaopen_task(te->L); lua_gc(te->L, LUA_GCRESTART, 0); if( LRT_LIB_OVERRIDE != 
NULL) LRT_LIB_OVERRIDE( te->L); OsUnlockMutex( tlMutex); if (init != NULL) { if 
(init[0] == '@') status = dofile( te->L, init+1); else status = dostring( te->L, 
init, "=LUA_INIT"); #if (_MSC_VER >= 1400) free(init); #endif } if (status == 0) 
{ if( te->fname[0] == '=' ) dostring( te->L, te->fname + 1, "=STRING_TASK"); 
else { if( LRT_DOFILE_OVERRIDE != NULL) LRT_DOFILE_OVERRIDE( te->L, te->fname); 
else dofile( te->L, te->fname); } } OsLockMutex( tlMutex, INFINITE); #ifndef 
NATV_WIN32 pthread_cleanup_pop( 0); #endif taskCleanup( te); OsUnlockMutex( 
tlMutex); return( 0); } 
Reply | Threaded
Open this post in threaded view
|

Re: RES: LuaTask needs your help

Stefan Sandberg
"*The Ends Justify The Means" :)*

Rafael - SosCpdTerra wrote:
Think the end justify the way.

-----Mensagem original-----
De: [hidden email]
[[hidden email]] Em nome de Kacper Wysocki
Enviada em: domingo, 8 de julho de 2007 08:41
Para: Lua list
Assunto: Re: LuaTask needs your help

On 7/7/07, Daniel Quintela <[hidden email]> wrote:
I'm almost ready to release LuaTask 1.6.2

I changed the way TASK_ENTRY memory is allocated.

I would appreciate it very much if you could test it and give me some
feedback.

You can get the candidate from: http://www.soongsoft.com/lua/ltask.c

I'd take a look but I can't reach this server...

-K


Reply | Threaded
Open this post in threaded view
|

RES: RES: LuaTask needs your help

Rafael - SosCpdTerra
;)


-----Mensagem original-----
De: [hidden email]
[[hidden email]] Em nome de Stefan Sandberg
Enviada em: domingo, 8 de julho de 2007 11:19
Para: Lua list
Assunto: Re: RES: LuaTask needs your help

"*The Ends Justify The Means" :)*

Rafael - SosCpdTerra wrote:
> Think the end justify the way.
>
> -----Mensagem original-----
> De: [hidden email]
> [[hidden email]] Em nome de Kacper Wysocki
> Enviada em: domingo, 8 de julho de 2007 08:41
> Para: Lua list
> Assunto: Re: LuaTask needs your help
>
> On 7/7/07, Daniel Quintela <[hidden email]> wrote:
>   
>> I'm almost ready to release LuaTask 1.6.2
>>
>> I changed the way TASK_ENTRY memory is allocated.
>>
>> I would appreciate it very much if you could test it and give me some
>> feedback.
>>
>> You can get the candidate from: http://www.soongsoft.com/lua/ltask.c
>>     
>
> I'd take a look but I can't reach this server...
>
> -K
>   



Reply | Threaded
Open this post in threaded view
|

Re: LuaTask needs your help

Daniel Quintela
In reply to this post by Duck-2
Duck escribió:

It seems to work across TASK_SLOTS_STEP boundaries when I use the test
code I had before. I tried creating 10 simultanous tasks, then 20, 40, 80,
160, 320 -- all good!
:-)

However, I can't create more than 509 tasks, i.e. 510 in total, including
the "_main_".

When I call task.create() for the 510th time, I get error -3 (can't create message queue). This happens with PTHREAD_STACK_SIZE set to 1m or to 2m. With the default PTHREAD_STACK_SIZE of 8m I get error -4 (can't create os thread) at the 383rd task, presumably because I have exhausted all 3g of virtual memory at that point.

Note that this isn't a new problem -- it exists with the 1.6.1 code as
well. I didn't realise it before because the segfault after
(TASK_SLOT_STEP-1) calls to task.create() got in the way :-) I just went
back to 1.6.1 and compiled with TASK_SLOT_STEP set to 600 in order to see
what happens: you get err -3 from the 510th call to task.create().
The real error is "EMFILE 24 /* Too many open files */" trying to create a pipe inside "QueCreate".
See your Linux limit with "ulimit -n".
Remember that each Queue needs one pipe == two file descriptors.

About the code itself, I have two suggestions.

Firstly, when you allocate space for the TASK_ENTRYs themselves (not the
list of pointers to TASK_ENTRYs), you don't check the return value of
malloc() for NULL.
You are right... but remember it is a "candidate". ;-)

Secondly, I'd strongly suggest factoring the actual malloc/realloc code
out into a function so it appears only once. At the moment you are
repeating code (except for a malloc() versus a realloc() -- and realloc()
behaves like malloc() when you feed it NULL). This means there are two
places to introduce errors or discrepancies.
Good idea.

My 2c.

Thanks,
Daniel


Reply | Threaded
Open this post in threaded view
|

Re: RES: LuaTask needs your help

Daniel Quintela
In reply to this post by Rafael - SosCpdTerra
Rafael - SosCpdTerra escribió:
Think the end justify the way.

-----Mensagem original-----
De: [hidden email]
[[hidden email]] Em nome de Kacper Wysocki
Enviada em: domingo, 8 de julho de 2007 08:41
Para: Lua list
Assunto: Re: LuaTask needs your help

On 7/7/07, Daniel Quintela <[hidden email]> wrote:
I'm almost ready to release LuaTask 1.6.2

I changed the way TASK_ENTRY memory is allocated.

I would appreciate it very much if you could test it and give me some
feedback.

You can get the candidate from: http://www.soongsoft.com/lua/ltask.c

I'd take a look but I can't reach this server...

-K
Sorry... without electric power in this neighborhood today from 11:20-UTC to 14:20-UTC...

:-(

Daniel


Reply | Threaded
Open this post in threaded view
|

RES: RES: LuaTask needs your help

Rafael - SosCpdTerra
If is ok to you Daniel, I put in here, while power down. 

http://www.soscpd.wiki.br/luataskhand/


-----Mensagem original-----
De: [hidden email]
[[hidden email]] Em nome de Daniel Quintela
Enviada em: domingo, 8 de julho de 2007 12:15
Para: Lua list
Assunto: Re: RES: LuaTask needs your help

Rafael - SosCpdTerra escribió:
> Think the end justify the way.
>
> -----Mensagem original-----
> De: [hidden email]
> [[hidden email]] Em nome de Kacper Wysocki
> Enviada em: domingo, 8 de julho de 2007 08:41
> Para: Lua list
> Assunto: Re: LuaTask needs your help
>
> On 7/7/07, Daniel Quintela <[hidden email]> wrote:
>   
>> I'm almost ready to release LuaTask 1.6.2
>>
>> I changed the way TASK_ENTRY memory is allocated.
>>
>> I would appreciate it very much if you could test it and give me some
>> feedback.
>>
>> You can get the candidate from: http://www.soongsoft.com/lua/ltask.c
>>     
>
> I'd take a look but I can't reach this server...
>
> -K
>   
Sorry... without electric power in this neighborhood today from 
11:20-UTC to 14:20-UTC...

:-(

Daniel



__________ NOD32 2384 (20070708) Information __________

This message was checked by NOD32 antivirus system.
http://www.eset.com




Reply | Threaded
Open this post in threaded view
|

Re: LuaTask needs your help

Daniel Quintela
In reply to this post by Thomas Lauer-3
Thomas Lauer escribió
This is too complicated, methinks. The old, simpler allocation scheme
was perfectly okay. The crashes resulted from invalid accesses, I think.
And I am not sure that these changes will solve all problems with that.
Yes, the problem arises from a illegal access to memory deallocated by the old reallocation scheme.
Following Kacper's clever analysis, I went through the ltask code step
by step. I finally changed just a few lines in taskthread(). This
produced a version that seems to run okay on my box. (I say "seems"
because the crashes I got were not strictly reproducible. I did
stress-test the code for a good while but with this sort of bug there's
always a small possibility that it's buggy but just refuses to crash.)

I didn't post these changes because I did other, unrelated changes as
well, so I couldn't just post a straightforward patch file. Also, I knew
you were already working on a fix anyway.

FWIW, the gist of my changes is the following. In the 1.6.1 version of
ltask.c, line 540, you initialise te:

te = aTask + ( ( long) vp);

The mutex is given up in line 554. After this line, the block aTask
points to can move at any time because of an realloc(). And if aTask
moves, then te is pointing nowhere.

However, in lines 558, 560, 567, 568, 571 ,573 and 583 te is still used.
(Getting the mutex back in 577 doesn't reinitialise te.)

I assume this is what produced the crashes I saw. When I changed the
code such that te is either not needed after line 554 or reinitialised
before accessing thread information everything seems to work fine (with
the caveat given).

Here is my current taskthread(). As I said, there are a few other
changes and I also compile this as C++ code. The three main changes are
commented:

[snipped code]

It was my first solution, but I preferred to add the pointer indirection to TASK_ENTRY blocks and keep its memory immutable.
Threads don't have to worry about its own memory.
Yes, it adds complexity and a level of indirection to access the task list... ¿what is the better approach?.
Sorry, probably should have posted this earlier. Anyway, HTH.
Don't apologize, "candidate time" is right for your feedback.

Thanks,
Daniel



Reply | Threaded
Open this post in threaded view
|

Re: RES: RES: LuaTask needs your help

Daniel Quintela
In reply to this post by Rafael - SosCpdTerra
Rafael - SosCpdTerra escribió:
If is ok to you Daniel, I put in here, while power down.
http://www.soscpd.wiki.br/luataskhand/
It seems to be broken...

Head of http://www.soscpd.wiki.br/luataskhand/ltask.c:

/* ** $Id: $ ** "Multitasking" support ( see README ) ** SoongSoft, Argentina ** ^M http://www.soongsoft.com [hidden email] ** Copyright (C) 2003-2007 ^M Daniel Quintela. All rights reserved. */ #include #include #include "lualib.h" ^M #ifdef _WIN32 # include # include # include #ifndef NATV_WIN32 #include #endif ^M #else # define _strdup strdup # define _strnicmp strncasecmp # include # include ^M # include # include # include # include # include #endif #define LT_NAMESPACE ^M "task" #include #include #include "syncos.h" #include "queue.h" #ifdef _WIN32 ^M static long ( __stdcall *LRT_LIB_OVERRIDE)( lua_State *L) = NULL; static long ( ^M
...


Regards,
Daniel


Reply | Threaded
Open this post in threaded view
|

Re: LuaTask needs your help

Thomas Lauer-3
In reply to this post by Daniel Quintela
Daniel Quintela <[hidden email]> wrote
> It was my first solution, but I preferred to add the pointer indirection 
> to TASK_ENTRY blocks and keep its memory immutable.
> Threads don't have to worry about its own memory.
> Yes, it adds complexity and a level of indirection to access the task 
> list... ¿what is the better approach?.

Well, I just prefer to keep stuff as simple as possible. I have not yet
fully analysed your changes; possibly they'll make LuaTask more robust
than the old allocation scheme.

What I know is that the old scheme *in itself* worked fine. I checked
with a pre-allocated array of 2048 entries and I had not a single crash
in a few hours of stress-testing. The culprit was clearly the realloc()
and that's why I concentrated on the code in taskthread().

But I am glad that this fixed now because LuaTask is a simple, but
elegant solution to getting threads into Lua.

-- 
cheers  thomasl

web : http://thomaslauer.com/start


Reply | Threaded
Open this post in threaded view
|

RES: RES: RES: LuaTask needs your help

Rafael - SosCpdTerra
In reply to this post by Daniel Quintela
I can't see the broken stuff. Think the best is wait for your server to come
up again.

-----Mensagem original-----
De: [hidden email]
[[hidden email]] Em nome de Daniel Quintela
Enviada em: domingo, 8 de julho de 2007 13:32
Para: Lua list
Assunto: Re: RES: RES: LuaTask needs your help

Rafael - SosCpdTerra escribió:
> If is ok to you Daniel, I put in here, while power down. 
>
> http://www.soscpd.wiki.br/luataskhand/
>   
It seems to be broken...

Head of http://www.soscpd.wiki.br/luataskhand/ltask.c:

/* ** $Id: $ ** "Multitasking" support ( see README ) ** SoongSoft, 
Argentina ** ^M
http://www.soongsoft.com [hidden email] ** Copyright (C) 
2003-2007 ^M
Daniel Quintela. All rights reserved. */ #include #include #include 
"lualib.h" ^M
#ifdef _WIN32 # include # include # include #ifndef NATV_WIN32 #include 
#endif ^M
#else # define _strdup strdup # define _strnicmp strncasecmp # include # 
include ^M
# include # include # include # include # include #endif #define 
LT_NAMESPACE ^M
"task" #include #include #include "syncos.h" #include "queue.h" #ifdef 
_WIN32 ^M
static long ( __stdcall *LRT_LIB_OVERRIDE)( lua_State *L) = NULL; static 
long ( ^M
...


Regards,
Daniel



__________ NOD32 2384 (20070708) Information __________

This message was checked by NOD32 antivirus system.
http://www.eset.com




Reply | Threaded
Open this post in threaded view
|

Re: RES: RES: LuaTask needs your help

Kacper Wysocki
On 7/8/07, Rafael - SosCpdTerra <[hidden email]> wrote:
I can't see the broken stuff. Think the best is wait for your server to come
up again.

Your attachment and the file on your website has had all its newlines
stripped off, rendering the code unreadable. Not even indent(1) can
deal with that...

Thankfully, Daniel's site http://www.soongsoft.com/lua/ltask.c is up again.
/_K

Reply | Threaded
Open this post in threaded view
|

Re: LuaTask needs your help

Kacper Wysocki
In reply to this post by Daniel Quintela
On 7/7/07, Daniel Quintela <[hidden email]> wrote:

I'm almost ready to release LuaTask 1.6.2

I changed the way TASK_ENTRY memory is allocated.

I would appreciate it very much if you could test it and give me some
feedback.

You can get the candidate from: http://www.soongsoft.com/lua/ltask.c

Hey Daniel,

good call on just resizing the array of task entries, instead of the
task entry structs themselves. That way the realloc won't affect the
running threads, and you save yourself a couple mutex locks. However,
you're still dereferencing the aTask pointer in tasklist(), getqhandle
and taskreceive() without grabbing the lock first, which will lead to
race conditions that are tough to pin down.

An example:
static int reg_tasklist(lua_State *L) {
   long i;
   lua_newtable( L);

   for( i = 0; i < countTask; i++)
       if( aTask[i]->running) { // <--- aTask pointer may have been
realloc()'ed elsewhere
           lua_pushnumber( L, i + 1);
           lua_newtable( L);

I suggest you lock down these aTask accesses... in reg_tasklist() you
will simply have to grab the mutex as you're iterating over the whole
task array,
but in taskreceive() and elsewhere,  if you want to avoid grabbing the
lock, you could push the task entry pointer (= aTask[i]) as a light
userdata upvalue for the lua apis OR as threaddata upon thread
creation.

I apologize for my laze in not producing a patch..
Cheers,
/K

Reply | Threaded
Open this post in threaded view
|

Re: LuaTask needs your help

Duck-2
In reply to this post by Daniel Quintela

I can't create more than 509 tasks, i.e. 510 in total

The real error is "EMFILE 24  /* Too many open files */"
trying to create a pipe inside "QueCreate"
See your Linux limit with "ulimit -n".

Yep, that's it. Sorry about the (uninformed) false alarm.

I increased the open files limit to 4096 and compiled with PTHREAD_STACK_SIZE set to 1m -- now I never see the error -3 (can't create queue, i.e. pipe), though I get an error -4 (out of memory) at a whisker below 3000 tasks, as expected.

I'm not sure how useful my testing is, since the concurrency involved and the behaviour of the tasks is very artificial. But what used to segfault reliably (if that doesn't sound like an oxymoron :-) now seems to work correctly, FWIW.

Out of interest, what made you drop TASK_SLOTS_STEP from 256 to 16? It means a whole lot of small heap objects when there are 100s or 1000s of tasks. Doesn't really matter on Lin/Win...just wondering. (And anyone who doesn't like it can change it.)


Reply | Threaded
Open this post in threaded view
|

Re: LuaTask needs your help

Thomas Lauer-3
In reply to this post by Kacper Wysocki
"Kacper Wysocki" <[hidden email]> wrote:
> good call on just resizing the array of task entries, instead of the
> task entry structs themselves. That way the realloc won't affect the
> running threads, and you save yourself a couple mutex locks. However,
> you're still dereferencing the aTask pointer in tasklist(), getqhandle
> and taskreceive() without grabbing the lock first, which will lead to
> race conditions that are tough to pin down.
> 
> An example:
> static int reg_tasklist(lua_State *L) {
>     long i;
>     lua_newtable( L);
> 
>     for( i = 0; i < countTask; i++)
>         if( aTask[i]->running) { // <--- aTask pointer may have been
> realloc()'ed elsewhere
>             lua_pushnumber( L, i + 1);
>             lua_newtable( L);

I am not sure that this line is indeed a problem. Isn't checking an int
an atomic operation? As aTask is declared volatile, it should be
re-referenced, so a change during the loop would not be a problem
either.

Further down in that function there are a couple of string copy ops, but
even these should be safe, I think.

-- 
cheers  thomasl

web : http://thomaslauer.com/start


Reply | Threaded
Open this post in threaded view
|

Re: LuaTask needs your help

David Given
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Thomas Lauer wrote:
[...]
>>         if( aTask[i]->running) { // <--- aTask pointer may have been
>> realloc()'ed elsewhere
[...]
> I am not sure that this line is indeed a problem. Isn't checking an int
> an atomic operation? As aTask is declared volatile, it should be
> re-referenced, so a change during the loop would not be a problem
> either.

Ah, but that's not an atomic operation. That line compiles into something like:

; r1 contains i
r0 = aTask            ; fetch aTask
r2 = r1 << 2          ; calculate offset into array
r0 = r0 + r2          ; calculate address of pointer in array
r0 = [r0+0]           ; dereference pointer to get address of structure
r0 = [r0+42]          ; fetch structure member

So if aTask gets modified while this code is in operation, suddenly you'll be
working with a stale pointer. Hilarity ensues.

- --
âââ ïïïïïïïïïïïïïï âââ http://www.cowlark.com âââââââââââââââââââ
â
â "Anything that makes people that angry is worth doing again." --- Scott
â Adams
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFGkgbYf9E0noFvlzgRAkloAJ0bgYIvuMj5lbcVuL443gJuev/N2ACgknjx
kCsc33edciRTwDJJsfrnACE=
=ULKu
-----END PGP SIGNATURE-----



Reply | Threaded
Open this post in threaded view
|

Re: LuaTask needs your help

Thomas Lauer-3
David Given <[hidden email]> wrote:
> > I am not sure that this line is indeed a problem. Isn't checking an int
> > an atomic operation? As aTask is declared volatile, it should be
> > re-referenced, so a change during the loop would not be a problem
> > either.
> 
> Ah, but that's not an atomic operation. That line compiles into something like:

You're right, of course. I was fixed on the comparison (which may indeed
be atomic) but the surrounding pointer arithmetic clearly isn't. I'll
have some more fun with fiddling the mutex then.

> Hilarity ensues.

Indeed;-)

-- 
cheers  thomasl

web : http://thomaslauer.com/start


12