Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Why not pass client handle in callbacks? #500

Open
snoopcatt opened this issue Jun 11, 2020 · 5 comments
Open

Why not pass client handle in callbacks? #500

snoopcatt opened this issue Jun 11, 2020 · 5 comments

Comments

@snoopcatt
Copy link

Evening.

I'm interested, why we don't pass client handle in read_start callbacks?
It will be very convenient when you using external functions:


local function callback(client, err, chunk)
print(type(client) == 'userdata') -- true
return 
end

client:read_start(callback) -- if we need to access client handle in callback by some reason, for now we need to define «one-liners»:
-- client:read_start(function(...) callback(client, ...) end)

So, why such behaviour isn't default?

@snoopcatt
Copy link
Author

I've just made simple patch to enable that behaviour for read_start, but I don't think it is correct solution btw.

--- stream.orig	2020-06-11 22:52:30.748014861 +0300
+++ stream.c	2020-06-11 22:52:35.524068945 +0300
@@ -93,21 +93,28 @@
   lua_State* L = data->ctx->L;
   int nargs;
 
+  // stream handle 1st arg
+  lua_newuserdata(L, sizeof(*handle));
+  lua_getfield(L, LUA_REGISTRYINDEX, "uv_stream");
+  lua_setmetatable(L, -2);
+  ++nargs;
+
   if (nread > 0) {
     lua_pushnil(L);
     lua_pushlstring(L, buf->base, nread);
-    nargs = 2;
+    nargs = nargs+2;
   }
 
   free(buf->base);
-  if (nread == 0) return;
+  if (nread == 0) 
+    return;
 
   if (nread == UV_EOF) {
-    nargs = 0;
+    // EOF
   }
   else if (nread < 0) {
     luv_status(L, nread);
-    nargs = 1;
+    ++nargs;
   }
 
   luv_call_callback(L, (luv_handle_t*)handle->data, LUV_READ, nargs);

@squeek502
Copy link
Member

Note that this is true for the bindings to all similar functions as well (e.g. udp_recv_start). Changing these functions to pass the stream/handle as the first parameter to the callbacks would be breaking changes, though, which we are trying to avoid. The way to do this in a non-breaking way would be to pass it as the last argument to the callbacks (not quite ideal, but don't have many options if we're trying to keep backwards compatibility).

In terms of the status quo, I think luv mostly expects the stream/handle to be able to be an upvalue of the callback function.

@snoopcatt
Copy link
Author

Maybe there is a better way to deal with it? ☺
3rd argument sounds much worse, because in different callbacks there are different (and sometimes variable) number of arguments.

So.
Maybe better variant is to plan that changes for next major release?

@SinisterRectus
Copy link
Member

We don't have any precedent for making breaking changes and we don't do arbitrary version bumps because the luv version is pegged to the libuv version.

@rphillips
Copy link
Member

This is likely not going to change since it would change the API. A different approach is use a bind like API like this.

ie:

bind(kls.someFunc, instance, client)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants