Welcome! Log In Create A New Profile

Advanced

[PATCH] BUG/MINOR: lua: schedule socket task when lua connect() is called

Posted by PiBa-NL 
Hi List, Thierry, Willy,

Created another little patch. Hope this one fits all submission criteria.

Regards,
PiBa-NL (Pieter)
From cc4adb62c55f268e9e74853f4a4893e2a3734aec Mon Sep 17 00:00:00 2001
From: PiBa-NL <[email protected]>
Date: Sat, 5 May 2018 23:51:42 +0200
Subject: [PATCH] BUG/MINOR: lua: schedule socket task upon lua connect()

The parameters like server-address, port and timeout should be set before
process_stream task is called to avoid the stream being 'closed' before it
got initialized properly. This is most clearly visible when running with
tune.lua.forced-yield=1.. So scheduling the task should not be done when
creating the lua socket, but when connect is called. The error
"socket: not yet initialised, you can't set timeouts." would then appear.

Below code for example also shows this issue, as the sleep will
yield the lua code:
local con = core.tcp()
core.sleep(1)
con:settimeout(10)
---
src/hlua.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/hlua.c b/src/hlua.c
index 4c56409..07366af 100644
--- a/src/hlua.c
+++ b/src/hlua.c
@@ -2423,6 +2423,10 @@ __LJMP static int hlua_socket_connect(struct lua_State *L)
WILL_LJMP(luaL_error(L, "out of memory"));
}
xref_unlock(&socket->xref, peer);
+
+ task_wakeup(s->task, TASK_WOKEN_INIT);
+ /* Return yield waiting for connection. */
+
WILL_LJMP(hlua_yieldk(L, 0, 0, hlua_socket_connect_yield, TICK_ETERNITY, 0));

return 0;
@@ -2582,8 +2586,6 @@ __LJMP static int hlua_socket_new(lua_State *L)
strm->flags |= SF_DIRECT | SF_ASSIGNED | SF_ADDR_SET | SF_BE_ASSIGNED;
strm->target = &socket_tcp.obj_type;

- task_wakeup(strm->task, TASK_WOKEN_INIT);
- /* Return yield waiting for connection. */
return 1;

out_fail_stream:
--
2.10.1.windows.1
Hi Pieter,

On Sun, May 06, 2018 at 12:00:14AM +0200, PiBa-NL wrote:
> The parameters like server-address, port and timeout should be set before
> process_stream task is called to avoid the stream being 'closed' before it
> got initialized properly. This is most clearly visible when running with
> tune.lua.forced-yield=1.. So scheduling the task should not be done when
> creating the lua socket, but when connect is called. The error
> "socket: not yet initialised, you can't set timeouts." would then appear.
>
> Below code for example also shows this issue, as the sleep will
> yield the lua code:
> local con = core.tcp()
> core.sleep(1)
> con:settimeout(10)

It seems to make sense indeed. I'll let Thierry check as he's the one who
really knows if there's a possible impact behind this.

Thanks,
Willy
> On 6 May 2018, at 06:32, Willy Tarreau <[email protected]> wrote:
>
> Hi Pieter,
>
> On Sun, May 06, 2018 at 12:00:14AM +0200, PiBa-NL wrote:
>> The parameters like server-address, port and timeout should be set before
>> process_stream task is called to avoid the stream being 'closed' before it
>> got initialized properly. This is most clearly visible when running with
>> tune.lua.forced-yield=1.. So scheduling the task should not be done when
>> creating the lua socket, but when connect is called. The error
>> "socket: not yet initialised, you can't set timeouts." would then appear.
>>
>> Below code for example also shows this issue, as the sleep will
>> yield the lua code:
>> local con = core.tcp()
>> core.sleep(1)
>> con:settimeout(10)
>
> It seems to make sense indeed. I'll let Thierry check as he's the one who
> really knows if there's a possible impact behind this.


I don’t see any impact, and I don’t remember the reason of the task woken
during init and not during connect().

I agree with Willy, this patch make sense.

In 1.6 version, the task is woken during the “hlua_socket_new” by the function
“stream_new”. I test your code and ... segfault :-) The bug fix for the version
1.6 and 1.7 are available in attachment. The version 1.8 was not affected.

Note that the following code:

>> local con = core.tcp()
>> core.sleep(1)
>> con:settimeout(10)

Can’t work in 1.6 and 1.7. It is bad :-( the function core.tcp() starts the task
without Lua requirement (this is done in “stream_new()” function) and the new
tcp connection disappear during the sleep(). I don’t see any work around. With
1.6, core.tcp() and conn:connect*() must be executed at the same time without
Lua yield :-(

The patch was not complex. I guess that I must remove the task from the run queue,
and apply your patch to wake up the task after connect. But the workaround is easy,
and I prefer to not modify this code. Any opinion about this ?

From the 1.8 version, the task_wakeup is no longer called in the function
“stream_new()”. The bug exists from the begining of the Lua socket, and it changes
to its new form from the commit:

87787acf724eeaf413393b5fce0047ad74356815
MEDIUM: stream: make stream_new() allocate its own task

So, I trust your patch, and it must be applied on the master branch and on the
1.8 branch.

BR,
Thierry
Attachments:
open | download - 0001-BUG-MEDIUM-hlua-sockets-Segfault-raises-if-the-socke.1.6.patch (1.2 KB)
open | download - 0001-BUG-MEDIUM-hlua-sockets-Segfault-raises-if-the-socke.1.7.patch (1.3 KB)
On Sun, May 06, 2018 at 02:33:46PM +0200, Thierry Fournier wrote:
> I agree with Willy, this patch make sense.

Thank you, now merged and backported.

> In 1.6 version, the task is woken during the "hlua_socket_new" by the function
> "stream_new". I test your code and ... segfault :-) The bug fix for the version
> 1.6 and 1.7 are available in attachment. The version 1.8 was not affected.

I've mentioned this in the 1.8 commit message so we don't lose it for older
versions.

Willy
Sorry, only registered users may post in this forum.

Click here to login