search -code with return statement does not stop proc execution

[bovine]

It seems that using a "return" command inside of the body of a -code block will not stop execution of a proc and immediately return that value back. This example demonstrates the problem for remotely mounted ctables, but it is believed to also affect certain variations of local or shmem ctables also.

package require birdclient

::birdclient::mount_speedtables

proc testret {} {
    set result "empty"
    inflight search -array row -limit 1 -code {
        set result "bad"
        return "good"
    }
    return $result
}
puts "return value is [testret]"

[resuna]

I seem to recall this coming up before. I'll have to dig into the code, but I think it was a particularly tricky sleight of hand in the case of client-server tables. Go ahead and assign it to me.

[lehenbauer]

There is a bug in Tcl_EvalObjv that it turns TCL_RETURN into TCL_OK. Tcl_EvalObjEx does not exhibit the problem.

On Apr 18, 2016, at 19:03, Peter da Silva notifications@github.com<mailto:notifications@github.com> wrote:

I seem to recall this coming up before. I'll have to dig into the code, but I think it was a particularly tricky sleight of hand in the case of client-server tables. Go ahead and assign it to me.

— You are receiving this because you were assigned. Reply to this email directly or view it on GitHubhttps://github.com/flightaware/speedtables/issues/55#issuecomment-211639476

[lehenbauer]

My remarks about Tcl_EvalObjv are incorrect. Simple attempts to replicate the problem so far do not. Continuing to investigate...

[lehenbauer]

I had the idea of sticking TclX’s “commandloop” in right before the “return” and using the interpreter to debug itself. Info frame lets you look at a stack frame if you specify the frame number so below you can see a walk up the stack to see what is calling what.

It looks to me that there are some procs generated by stapi called ::stapi::shared3::handler and stapi::shared3::reader and the superbird class search method, all of which have to properly cause the tcl return code to make it back and one or more of which probably isn’t.

pepbi stbug % source foo.tcl 
row(_key)         = EDLV:NRN
row(bts_code)     = 
row(city)         = Weeze
row(citystate)    = Weeze
row(country_code) = DE
row(elevation)    = 106.0
row(iata)         = NRN
row(icao)         = EDLV
row(latitude)     = 51.602414
row(longitude)    = 6.142172
row(name)         = Weeze
row(state)        = 
row(timezone)     = :Europe/Berlin
row(wiki_url)     = http://en.wikipedia.org/wiki/Weeze_Airport
returning NRN
%info frame
17
%info frame 17
type eval line 1 cmd {info frame 17} proc ::stapi::shared3::handler
%info frame 16
type eval line 4 cmd commandloop proc ::stapi::shared3::handler
%info frame 15
type eval line 1 cmd {::stapi::shared3::reader search -compare {{= icao EDLV}} -limit 1 -array_with_nulls row -code {
        parray row
        puts "returning $row(iata)"
commandloop
        return $row(iata)
    }} proc ::stapi::shared3::handler
%info frame 14
type proc line 6 cmd {uplevel 1 [namespace which reader] $args} proc ::stapi::shared3::handler
%info frame 13
type eval line 1 cmd {::stapi::shared3::handler search -compare {{= icao EDLV}} -limit 1 -array_with_nulls row -code {
        parray row
        puts "returning $row(iata)"
commandloop
        return $row(iata)
    }} method search class ::SuperBird
%info frame 12  
type proc line 5 cmd {uplevel $command} method search class ::SuperBird
%info frame 11
type eval line 1 cmd {::superbird::all_airport search -compare {{= icao EDLV}} -limit 1 -array_with_nulls row -code {
        parray row
        puts "returning $row(iata)"
commandloop
        return $row(iata)
    }} proc ::superbird
%info frame 10
type source line 266 file /usr/local/flightaware/vhosts/production/fa_web/packages/flightaware-gadgets/superbird-tables.tcl cmd {uplevel [linsert $args 0 $obj $action]} proc ::superbird
%info frame 9
type source line 7 file /usr/home/karl/src/stbug/foo.tcl cmd {superbird search all_airport -compare [list [list = icao "EDLV"]] -limit 1 -array_with_nulls row -code {
        parray row
        puts "returning $row(iata)"
commandloop
        return $row(iata)
    }} proc ::testmoo level 0
%info frame 8
type eval line 1 cmd testmoo proc ::tclreadline::Loop

[lehenbauer]

Where is the code backing the proc created here $prop(type) create ${ns}::reader reader $handle at https://github.com/flightaware/speedtables/blob/master/stapi/client/shared.tcl#L61

[lehenbauer]

Here is the shared handler proc body. It gets generated into namespaces containing stapi instances. Are the uplevels here being mishandled?

[mutability]

If this is a remote ctable I would look suspiciously at remote_ctable_invoke / remote_ctable_send; I don't see a path that lets you return something other than a simple value or an error there.

[mutability]

In particular this looks suspicious: https://github.com/flightaware/speedtables/blob/dfbcc74e4c9aa457ecc0279550ba0403e96703dc/ctable_server/ctable_client.tcl#L375

[lehenbauer]

Yeah @bovine and I were looking at that yesterday. It definitely needs to be return -code $status $result

[bovine]

@mutability My commit 71ca885 in the "retval" branch fixes that line, but it doesn't seem to fix the broken behavior in the example at the top of this issue.

[resuna]

Return should be propagated. I don't think break should, because we don't want to break an outer loop.

On 2016-04-20, at 10:19, Karl Lehenbauer notifications@github.com wrote:

Yeah @bovine and I were looking at that yesterday. It definitely needs to be return -code $status $result

� You are receiving this because you commented. Reply to this email directly or view it on GitHub

[bovine]

@resuna Can you add testcases for these various combinations (return, break, etc) for remote, local, shmem tables?

[resuna]

in the "retval" branch?

[bovine]

Make a new branch. Karl says he has found the problems and will be committing his fixes in his own branch. I'll delete my retval branch once he does his.

[resuna]

OK, I'll wait for all this to get settled down first.

[bovine]

This commit was included in 1.9.4

[bovine]

resuna says the testcases for this are in speedtables/ctables/tests/stapireturn.tcl