passing a promises resolve callback to an event callback in the oposite direction looses context

[tommyvn]

I'm on the nginx dockerhub builds of 1.21.6 mainline and 1.22.0 stable, I tried on both and got the same result.

I have an upstream TCP service that wants a handshake. depending on this handshake, it wants the proxy to forward some or all of the protocol header. a good example is some upstreams want a proxy protocol header, others do not, and the handshake tells me this, but there are other things I strip from the protocol handshake so it's not only this.

If I hard code any given filter ahead of time I am indeed able to filter my protocol for my upstreams, so this part at least is not impossible, it is only the dynamic nature of the handshake that I am struggling with.

it is something like this:

sequenceDiagram
    client->>proxy: initial protocol data
    proxy->>upstream: handshake
    upstream->>proxy: filter
    proxy->>upstream: initial protocol data with filter applied
    upstream-->>client: passthru
    client-->>upstream: passthru

Loading

to achieve this I tried to pass a promise resolve callback from my upload event into a download event and then await it (and also tried a .then(...), same issue).
The problem I'm having is that my upload event handler switches it's s object to the download event handlers context after my await returns, after the download handler calls the passed resolve function from my promise. It seems that whoever calls resolve takes over the context for anyone awaiting. The most concise way I could find to illustrate this is by forcing an exception. immediately before awaiting this object my exception claims to be in the upstream direction, but immediately after it claims to be in the client direction.

Is it possible to fix my promise to achieve my handshake sequence diagram, or really are there any tricks to get that handshake over from the download to the upload handler?

Here is a stream filter function that illustrates my issue with forced exceptions:

function filter(s) {
  async function handleUpload (data, flags) {
    s.send('\x00\x00\x17yolo-test.dev.localhost\x00P', flags);
    const p = new Promise(( resolve, reject ) => {
      s.on("download", prepareHandleDownload(resolve));
    });
    s.off("upload");
    s.on("upload", handleUploadPhase2);
    // s.error(e);  // <=== if I uncomment this I get "e" is not defined while proxying and sending to upstream in my error log
    await p;
    // s.error(e);  // <=== if I uncomment this I get "e" is not defined while proxying and sending to client in my error log
  };
  function handleUploadPhase2 (data, flags) {
        s.error("handleUpload");
    };

  function prepareHandleDownload(resolve) {
    function handleDownload(data, flags) {
      s.off("download");
      resolve("yolo");
    };
    return handleDownload;
  };

  s.on("upload", handleUpload);
}

[xeioex]

hi @tommyvn,

The promises and async functions are just recently introduced and are not well tested with stream module (unlike http module).
I would suggest to start from pure js callbacks.

[xeioex]

Hi @tommyvn,

Take a look at the following patch.

[tommyvn]

This change is in njs v0.7.7, yes?

A quick glance suggests that 569292e is indeed in that tag, so I'm assuming yes, in which case it is still not working for me :( s.send still looses the direction context after an await in 0.7.7.

I've updated my example to better reflect my sequence diagram above, request, handshake to upstream, response from upstream, and send.
I have a commented out a s.send call before the await, if I un-comment this call I see the data arrive on my upstream as expected, but the s.send call after the await behaves as if called in the download callback and is instead sent back to the client.

The njs version logged out below is indeed 0.7.7 BTW running under nginx 1.23.1

function filter(s) {
  s.error(`using njs ${njs.version}`);
  //handshakeDownload
  function prepareHandshakeDownload(resolve) {
    function handleDownload(data, flags) {
      s.error(`download: data=${JSON.stringify(data)}`);
      s.off("download");
      resolve(data);
    };
    return handleDownload;
  };

  //handshakeUpload
  s.on("upload", async (data, flags) => {
    s.send("fake handshake", flags);
    const p = new Promise(( resolve, reject ) => {
      s.on("download", prepareHandshakeDownload(resolve));
    });
    s.off("upload");
    // s.send(data, flags);  // In this call I see data arrive on my upstream as expected
    const handshakeResponse = await p;
    s.error(`upstream: handshakeResponse=${JSON.stringify(handshakeResponse)}`);
    s.error(`upstream: s.send(${JSON.stringify(data)})`);
    s.send(data, flags);  // In this call I see data arrive on my downstream, the direction has flipped after the await
  });
}

I also tried doing this in a non-async using a p.then callback on my promise, and same result. Before the direction is correct, but after the promise it flips.

I think that my issue is different to the ngx.fetch callback. that one is still happening within the context of a single direction, while mine tries to use a promise to "pause" a callback in one direction until the other tells it to continue, and it seems that it's this switch that flips the s.send method over to the other direction. I'm guessing that that `s.send is context dependent, and the promise somehow moves it over to the download direction, but my C skills are non-existent so please read this as the guess that it is.

if you want to see this in action create a stream server that looks like this:

  server {
    listen 20000;

    js_filter testAsync.filter;
    proxy_pass localhost:20001;
  }

fire up a socat on port 20001 socat TCP-LISTEN:20001,reuseaddr -, and connect another to port 20000 socat - tcp:localhost:20000, I then send "request" in my client, i see "fake handshake" on my server, i send "response" from my server, and I see the original "request" in my client instead of my server.

[xeioex]

@tommyvn

As of 0.7.7 you can decide which was to send using from_upstream flag which is a property of flags argument.
I am considering adding more straightforward methods like s.sendUpstream() and s.sendDownstream().

But, before I do, can you confirm that from_upstream is working as expected for your use case?

So, if you intend to send to a client, from_upstream should be true when s.send() is invoked.

[tommyvn]

so, changing the last few lines of my example like so?

    const newFlags = {
      flush: flags.flush,
      last: flags.last,
      from_upstream: false, // I tried this as true also
    }
    s.error(`upstream: s.send(${JSON.stringify(data)}, ${JSON.stringify(newFlags)})`);
    s.send(data, newFlags);  // In this call I see data arrive on my downstream, the direction has flipped after the await
  });
}

if so, it didn't work I'm afraid to report. I tried both true and false in that from_upstream field but both result in the same behavior, as in the direction of the send after the await is always upload download (sorry, mistyped this in my first reply, it's always sent in download direction, corrected in all places i mixed this up), regardless of this setting.

I also tried moving the s.send call into the callback I pass in to the s.on("download" callback so that the code ran in the download context rather than in the upload context after the await, and same result, regardless of from_upstream being true or false it always sends in the upload download direction.

I also tried switching my upload/download to upstream/downstream callbacks and got the same unexpected result if that makes a difference.

Have you tried this filter yourself? did you see different results? that exact filter in a stream server is the one I'm using + those two socat commands, i've got no other tricks going on, it's as simple as this. I send req into the client side socat, i see fake handshake pop out the server side socat, i then send res on the server side socat (to emulate a handshake reply), this is where I'd expect the handshake to complete in the filter and the promise callback to then send the initial client data of req to the server socat, but instead this req is always sent back to the client socat, as if a plain s.send were called in the upload callback.

If it helps I could try set up a Dockerfile that demonstrates this issue on the debian version of the nginx container? I pull and build the 0.7.7 njs module in my Dockerfile using the repos mentioned at http://nginx.org/en/linux_packages.html#Debian and I'm starting to wonder if maybe somehow this source is different to what you're expecting to be in a 0.7.7 build despite the njs.version claiming it is the correct version. It's difficult to tell because (I did a strings ngx_stream_js_module.so | grep from_upstream and it's got text that was added in 0.7.7, so it seems to be the right code it was built from). calls to s.send with bogus flags don't fail, it seems to silently ignore flags that don't exist, so I'm not 100% certain that the from_upstream is even seen by the call, it doesn't throw an error on the previous version of njs when I send that or even made up flags in ether.

BTW unambiguous methods like s.sendUpstream and s.sendDownstream sound great to me. if I were calling s.sendUpstream and seeing the data appear on my downstream then I'd know 100% that it were a bug, but now honestly I'm not sure if I'm doing something wrong, mispelling the flag, or if it really is a bug.

[xeioex]

This change is in njs v0.7.7, yes?

A quick glance suggests that 569292e is indeed in that tag, so I'm assuming yes, in which case it is still not working for me :( s.send still looses the direction context after an await in 0.7.7.

I've updated my example to better reflect my sequence diagram above, request, handshake to upstream, response from upstream, and send. I have a commented out a s.send call before the await, if I un-comment this call I see the data arrive on my upstream as expected, but the s.send call after the await behaves as if called in the download callback and is instead sent back to the client.

The njs version logged out below is indeed 0.7.7 BTW running under nginx 1.23.1

function filter(s) {
  s.error(`using njs ${njs.version}`);
  //handshakeDownload
  function prepareHandshakeDownload(resolve) {
    function handleDownload(data, flags) {
      s.error(`download: data=${JSON.stringify(data)}`);
      s.off("download");
      resolve(data);
    };
    return handleDownload;
  };

  //handshakeUpload
  s.on("upload", async (data, flags) => {
    s.send("fake handshake", flags);
    const p = new Promise(( resolve, reject ) => {
      s.on("download", prepareHandshakeDownload(resolve));
    });
    s.off("upload");
    // s.send(data, flags);  // In this call I see data arrive on my upstream as expected
    const handshakeResponse = await p;
    s.error(`upstream: handshakeResponse=${JSON.stringify(handshakeResponse)}`);
    s.error(`upstream: s.send(${JSON.stringify(data)})`);
    s.send(data, flags);  // In this call I see data arrive on my downstream, the direction has flipped after the await
  });
}

I also tried doing this in a non-async using a p.then callback on my promise, and same result. Before the direction is correct, but after the promise it flips.

I think that my issue is different to the ngx.fetch callback. that one is still happening within the context of a single direction, while mine tries to use a promise to "pause" a callback in one direction until the other tells it to continue, and it seems that it's this switch that flips the s.send method over to the other direction. I'm guessing that that `s.send is context dependent, and the promise somehow moves it over to the download direction, but my C skills are non-existent so please read this as the guess that it is.

if you want to see this in action create a stream server that looks like this:

  server {
    listen 20000;

    js_filter testAsync.filter;
    proxy_pass localhost:20001;
  }

fire up a socat on port 20001 socat TCP-LISTEN:20001,reuseaddr -, and connect another to port 20000 socat - tcp:localhost:20000, I then send "request" in my client, i see "fake handshake" on my server, i send "response" from my server, and I see the original "request" in my client instead of my server.

Hi @tommyvn,

Thanks for the reproducible report,

Now, I see the reason. Currently, from_upstream does not have a precedence over invocation context.
If s.send() is called in a synchronous context, the current direction will overrule the from_upstream (i.e.it will be ignored).
The patch below always uses from_upstream IF it is provided.

Could you, please, test the following patch?

# HG changeset patch
# User Dmitry Volyntsev <xeioex@nginx.com>
# Date 1663812395 25200
#      Wed Sep 21 19:06:35 2022 -0700
# Node ID 1ecf7d6b0773012ba763f3991203a28b57d0e380
# Parent  74d30c2d70f37729227c433950cb583becfa1fea
[mq]: 552

diff --git a/nginx/ngx_stream_js_module.c b/nginx/ngx_stream_js_module.c
--- a/nginx/ngx_stream_js_module.c
+++ b/nginx/ngx_stream_js_module.c
@@ -1288,6 +1288,7 @@ static njs_int_t
 ngx_stream_js_ext_send(njs_vm_t *vm, njs_value_t *args, njs_uint_t nargs,
     njs_index_t unused)
 {
+    int                    from_upstream;
     unsigned               last_buf, flush;
     njs_str_t              buffer;
     ngx_buf_t             *b;
@@ -1339,6 +1340,8 @@ ngx_stream_js_ext_send(njs_vm_t *vm, njs
 
     flags = njs_arg(args, nargs, 2);
 
+    from_upstream = -1;
+
     if (njs_value_is_object(flags)) {
         value = njs_vm_object_prop(vm, flags, &flush_key, &lvalue);
         if (value != NULL) {
@@ -1349,6 +1352,15 @@ ngx_stream_js_ext_send(njs_vm_t *vm, njs
         if (value != NULL) {
             last_buf = njs_value_bool(value);
         }
+
+        value = njs_vm_object_prop(vm, flags, &from_key, &lvalue);
+        if (value != NULL) {
+            from_upstream = njs_value_bool(value);
+        }
+
+        if (value == NULL && ctx->buf == NULL) {
+            goto exception;
+        }
     }
 
     cl = ngx_chain_get_free_buf(c->pool, &ctx->free);
@@ -1371,23 +1383,13 @@ ngx_stream_js_ext_send(njs_vm_t *vm, njs
     b->pos = b->start;
     b->last = b->end;
 
-    if (ctx->buf != NULL) {
+    if (from_upstream == -1) {
         *ctx->last_out = cl;
         ctx->last_out = &cl->next;
 
     } else {
-        if (!njs_value_is_object(flags)) {
-            goto exception;
-        }
-
-        value = njs_vm_object_prop(vm, flags, &from_key, &lvalue);
-        if (value == NULL) {
-            goto exception;
-        }
-
-        if (ngx_stream_js_next_filter(s, ctx, cl, njs_value_bool(value))
-            == NGX_ERROR)
-        {
+
+        if (ngx_stream_js_next_filter(s, ctx, cl, from_upstream) == NGX_ERROR) {
             njs_vm_error(vm, "ngx_stream_js_next_filter() failed");
             return NJS_ERROR;
         }

[tommyvn]

with your patch i can now send traffic in both directions from any one directions callback, which enables the bi-directional handshakes I have in my original sequence diagram, it worked 💪

I've checked from_upstream: false both before and after the await in the upload callback, and also in the download callback, and the direction is now consistently to the upstream server when false, and the client when true, regardless of where in the code it is.

[xeioex]

Hi @tommyvn,

Feel free to test the following patch set: https://gist.github.com/xeioex/5fbb0f581b4b2872c63b6aa5105f6c9b

the second patch adds s.sendUpstream() and s.sendDownstream() they are identical to s.send() except they always send data in the appropriate direction.