luvit / luv

Bare libuv bindings for lua
Apache License 2.0
809 stars 183 forks source link

Why not pass client handle in callbacks? #500

Open snoopcatt opened 4 years ago

snoopcatt commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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)