starwing / lua-protobuf

A Lua module to work with Google protobuf
MIT License
1.74k stars 387 forks source link

Is lua-protobuf thread-safe / LuaLanes safe? #237

Closed ewmailing closed 1 year ago

ewmailing commented 1 year ago

Hi, I'm wondering if lua-protobuf is able to be used with LuaLanes. I started using lua-protobuf with LuaLanes and sometimes I get mysterious crashes. I don't know if lua-protobuf is the reason for the crashes, but in some testing, my crashes stopped when I removed lua-protobuf. (But since the crashes only happen sometimes, I don't know if this is coincidence.)

The first place in lua-protobuf that I wonder about is the protoc module. I use protoc.load like the example in the Readme.md. Since protoc.load seems to affect things globally and doesn't get its own context that you pass with calls to pb, I'm wondering if there is shared global state here that is dangerous when called simultaneously across different LuaLanes.

Thank you

starwing commented 1 year ago

You could try compiling proto file to .pb file (using the official protoc executable and -o flag) first.

Then, try use protoc.new(), it doesn't need global state.

Thirdly, try pb.state(). Use separate state in one thread.

Other parts of lua-protobuf do not modify global state.

ewmailing commented 1 year ago

Thank you for the response. I am confused by the relationship of protoc and pb because the APIs never directly connect to each other (e.g. protoc doesn't return any parameters that you pass to pb functions).

If I compile a proto file to .pb (e.g. protoc --descriptor_set_out MyFoo.pb MyFoo.proto), does that let me avoid needing to use protoc?

    local fh, err = io.open("MyFoo.pb", "rb")
    local file_data = fh:read("*all")
    local pb = require("pb")
    pb.load(file_data)

Is this what you were suggesting with your first sentence? Then I wouldn't need to use protoc at all, right? And so I wouldn't need protoc.new(), right?

FYI, each LuaLane creates a new lua_State to run in. So each pb probably gets its own state already, so I don't think I need pb.state() in my case.

Thank you

starwing commented 1 year ago

If I compile a proto file to .pb (e.g. protoc --descriptor_set_out MyFoo.pb MyFoo.proto), does that let me avoid needing to use protoc?

Yes.

Is this what you were suggesting with your first sentence? Then I wouldn't need to use protoc at all, right? And so I wouldn't need protoc.new(), right?

Yes.

FYI, each LuaLane creates a new lua_State to run in. So each pb probably gets its own state already, so I don't think I need pb.state() in my case.

Okk, that make sense.

ewmailing commented 1 year ago

I haven't gotten any crashes since I made the change to load the .pb and avoid protoc. :+1:

I'm just curious, what happens if I pb.load the same descriptor multiple times in the same state? Is this harmless? Is there a reloading cost or does the system know it is already loaded and returns early?

ewmailing commented 1 year ago

I spoke too soon. My crashes came back :(

I was looking at the source code in pb.c. There is a global variable called global_state:

https://github.com/starwing/lua-protobuf/blob/449ad5d66c89cb408c0f839883874f8d409e3cf2/pb.c#L131

My current hunch is that this may be related to my crashes while using Lanes. I'm thinking I want to try changing this to not use a global variable, to see if it will make my crashes go away. Can you give me any hints on what is involved to change this?

Thanks

starwing commented 1 year ago

this global just used in pb.state() routines, you could delete it (and relate codes) and see whether it works.

ewmailing commented 1 year ago

My crashes disappeared for a couple of weeks, but just came back. I tried deleting global_state (and related code), but it didn't fix the problem. So that was a red herring for me. So now, I think my problem might be caused by something unrelated (I hate threading bugs). But I think this part of lua-protobuf is fine and not the problem. Thanks for your help and time.