Error when calling non-closed local server functions

jamescheney commented 2 years ago

Thanks to #1133 it is now possible to write local / anonymous functions that have client or server annotations. Such functions need not be closed, for example:

fun foo(x) client { replaceNode(
    (fun (y) server  {<div id="xyz"><p><b>{stringToXml(x)}</b>:<b>{stringToXml(y)}</b></p></div>})("xyzzy")
    , getNodeById("xyz"))
}

fun testPage(msg) {
  page
  <html>
    <body>
      <div>Message: {stringToXml(msg)}</div>
        <form l:onsubmit="{foo(x)}" method="post">
          <input type="text" l:name="x"/>
    <button type="submit">Submit!</button>
        </form>
      <div id="xyz">This should be replaced</div>
    </body>
  </html>
}

fun main () {
  addRoute("",testPage);
  servePages()
}

main()

However, running this and clicking the Submit button yields (I think) the same error @kwanghoon reported in a comment on #907, adjusting for changes:

Uncaught Error: Fatal error: call to server returned an error. Details: ***: Error: File "core/evalir.ml", line 252, characters 18-24: Assertion failed

This error appears in the browser console, but relays an error raised on the server.

Removing the server label from the anonymous function yields correct behavior, presumably because the function is called on the client and it is closure converted correctly there.

Replacing foo with the following:

fun foo(x) client { replaceNode(
    (fun (x,y) server {<div id="xyz"><p><b>{stringToXml(x)}</b>:<b>{stringToXml(y)}</b></p></div>})(x,"xyzzy")
    , getNodeById("xyz"))
}

so that the anonymous server function is lambda-lifted in place, yields correct behavior.

The error is being raised here: https://github.com/links-lang/links/blob/474137f31a43d7589d2711968d8a564bb14c173f/core/evalir.ml#L252

which is an "unreachable" case of a pattern match that tries to match up the closure variable and environment. Evidently these are getting out of sync somehow, possibly due to an unhandled case or no-longer-guaranteed invariant in closure conversion. Through random hacking I found that the change in commit 8cee258 "fixes" this problem. But this is almost certainly not the right way to fix it, the right way is to make sure that the function info and environment/argument list do not get out of sync in the first place.

kwanghoon commented 2 years ago

Hello @jamescheney,

In Links, CPS-converted functions are thought to take its environment as their first argument.

When the example program calls a server function with a text "zzzzz", cgi_args is as follows:

   __name: "MjQwMQ=="                                              ===> 2401
   __args: "eyIxIjp7IjIzOTYiOiJ6enp6eiJ9LCIyIjoieHl6enkifQ=="      ===> {"1":{"2396":"zzzzz"},"2":"xyzzy"}
   __env: "e30="                                                   ===> {}
   __client_id: "Y2lkXzA="                                        ===> cid_0

A quick workaround is to change webif.ml

to construct a FunctionPtr with Some (hd args) as its environment, and
to omit the first argument from the arguments by tl args.

https://github.com/links-lang/links/blob/474137f31a43d7589d2711968d8a564bb14c173f/core/webif.ml#L44

    let func,args =
      match fvs with
      | `Record [] -> let i_fname = int_of_string fname in
          if Lib.is_primitive_var i_fname
          then `PrimitiveFunction (Lib.primitive_name i_fname, Some i_fname),args
          else match hd args with
               | `Record _ -> `FunctionPtr (int_of_string fname, Some (hd args)), tl args
               | _ -> `FunctionPtr (int_of_string fname, None), args
      | _ -> `FunctionPtr (int_of_string fname, Some fvs), args
    in
    RemoteCall (func, valenv, args)

jamescheney commented 2 years ago

Thanks. As I understand it, thanks to closure conversion the environment part of the call is supposed to always be empty (there is even a comment in jslib suggesting to get rid of it). So like my workaround mentioned above this seems to fix the symptom but not the root cause of the problem.

I think the issue is that after closure conversion, when we call the server from the client we look up information about the function's arguments and closure variable (if any) using find_fun, and this information is inconsistent with the closure converted version: a closure variable is present, but the environment is being passed as an ordinary argument. My workaround handles that case by patching the argument list with the closure variable if there is one but no environment was passed.

I think the right way to do this is fix the dictionary used by fun_info so that after closure conversion is finished, the closure variable are all added to the argument list, but haven't had the time to look at what is currently done to see if this actually makes sense.

kwanghoon commented 2 years ago

Thanks for the explanation.

One thing still strange to me is that in the web interface, only FunctionPtr with None as the second argument is created if the environment part of the remote call is supposed to always be empty (i.e., Record []), whatever the closure variable (z) is. I don't know what I am missing.

https://github.com/links-lang/links/blob/474137f31a43d7589d2711968d8a564bb14c173f/core/webif.ml#L31-L52

jamescheney commented 2 years ago

It seems in the server-side IR, after closure conversion things are still represented as "closures", but the function bodies refer to the environment through the closure variable defined in the functions fun_def entry. Thus, the IR code isn't changed so that the IR manages the environment itself, instead whenever a closure is created the current environment is stored in it and when a call happens, that environment is passed in bound to the closure variable, if any. However, when compiling code to the client for server calls we can't do this because we are generating really-closed JS functions.

It looks like Record [] is being used to encode NONE, since environments / closure variables only get created if the function had some local variable references.

I suspect a reason it'd be annoying to fully closure-convert the IR code (i.e. make the closure variable a normal parameter, and rewrite calls/closure creation to manage the environment using IR code rather than having the interpreter do it) is that IR typing would become more complicated: functions with the same input/output types but different environments would have different types. This problem can be solved by adding existential types so that the type of a closure-converted function can be something like "exists Env. ((Env,t1,...,tn) -> t , Env)" but we currently don't have this even in the IR. That being the case, since the mismatch seems to arise at the point where we create a FunctionPtr for a server-side call decoded from a client request, I think my suggestion is: when the server-side function has a closure variable, but the environment provided is empty, pop off the first parameter and treat it as the environment. My fix does the same thing but in a much more mysterious way (and one which, if there were other bugs leading to mismatches between the function info and optional closure in server side code, would potentially lead to other hard-to-debug situations).

jamescheney commented 2 years ago

Having looked at some of the generated IR to see how the above two examples differ, I came up with a smaller example that seems to result in the same problem. The following code works:

mutual {
fun testPage(msg) {
  page
  <html>
    <body>
    <div>Message: {stringToXml(msg)}</div>
      <div id="x">
        <p>GAGA</p>
      </div>
      <a href="" l:onclick="{changeContent()}">Click me</a>
    </body>
  </html>
}

fun snippet (time,testPage) server {
        <a l:href="{testPage(intToString(time))}">href handled OK</a>
}

fun changeContent() client {
  var time = clientTime();
  replaceNode(snippet(time,testPage),getNodeById("x"))
}
}

fun main () {
  addRoute("",testPage);
  servePages()
}

main()

Here, I've modified snippet to get rid of the time but also be completely closed, i.e. not refer to the other functions in the same mutual block.
This version where snippet is lifted out of the mutual block however fails with the same error as above:

fun snippet (testPage) server {
        <a l:href="{testPage("42")}">href handled OK</a>
}

mutual {
fun testPage(msg) {
  page
  <html>
    <body>
    <div>Message: {stringToXml(msg)}</div>
      <div id="x">
        <p>GAGA</p>
      </div>
      <a href="" l:onclick="{changeContent()}">Click me</a>
    </body>
  </html>
}

fun changeContent() client {
  replaceNode(snippet(testPage),getNodeById("x"))
}
}

fun main () {
  addRoute("",testPage);
  servePages()
}

main()

I think what may be happening is that for some reason, the call to testPage in the second version is being attempted as a client call, which is of course silly and unnecessary. But even by annotating testPage as server in the second version the same error still occurs, which suggests that somewhere a client closure is being incorrectly created.

jamescheney commented 2 years ago

Hmm. So changing the working version to:

mutual {
fun testPage(msg) {
  page
  <html>
    <body>
    <div>Message: {stringToXml(msg)}</div>
      <div id="x">
        <p>GAGA</p>
      </div>
      <a href="" l:onclick="{changeContent()}">Click me</a>
    </body>
  </html>
}

fun snippet (time,f) server {
        <a l:href="{f(intToString(time))}">href handled OK</a>
}

fun changeContent() client {
  var time = clientTime();
  replaceNode(snippet(time,testPage),getNodeById("x"))
}
}

fun main () {
  addRoute("",testPage);
  servePages()
}

main()

makes it fail again. So it seems that there may be another issue, relating to scoping/resolution of names in mutual blocks.

jamescheney commented 1 year ago

This issue seems to have been worked around by #1138. I think it should be closed, and another issue collecting ideas for a more principled design created.

Some of the examples in this page still encounter the "Error: Cannot call client side function '_$ClosureTable.apply' because of before server page is ready" problem also covered by #1134, but I think that is a separate problem - the examples there illustrate that the problem can occur with local server-annotated functions that are closed.

links-lang / links

Error when calling non-closed local server functions #1136