facebook/fbthrift

go lib bugs for concurrent server

lee-qiu opened this issue · 6 comments

lee-qiu commented

I find 2 problems during using concurrent server of thrift go lib:

  1. the process routine maybe panic when socket.Write() function and socket.Close() function called in parallel. details is that: socket.IsOpen() in Write() -> socket.Close() -> socket.pushDeadline() in Write() panic. this situation can be produced easily by closing client socket with pending requests.

  2. the process routine didn't handle socket write error when connection is broken and it will produce lots of SIGPIPE(there are huge responses to write socket). it shouldn't be a problem considering that SIGPIPE will be ignored in go, but it makes my program crashed with SIGSEGV.
    the root cause is that I used cgo(fbthrift too) in my program which register SIGPIPE handler without SA_ONSTACK flag, I guess too many SIGPIPE makes cpp stack overflow, then I added SA_ONSTACK flag for SIGPIPE handler and the result is fine(seems this has been fixed in main branch), but I think stop writing on a broken connection is a better way.

I tried to fix them locally but can't push them to this repo, so I am wondering that are you willing to treat them as bugs and may I fix them in this repo? waiting for your reply sincerely.

lee-qiu commented

@yfeldblum could you please take a look at this?

awalterschulze commented

This is a known issue that we need to address. The current ConcurrentServer is buggy in various ways, especially in the promise of handling concurrent requests. We plan to replace it with a new version that can handle a lot more of these cases that one would expect a thrift server to handle.

Until then, we are going to delete ConcurrentServer to limit the damage of anyone else using it and also running into these bugs. There is going to be a bit of a delay in fixing this properly, but I hope it will be worth the wait.

  • 👍1
lee-qiu commented

Thanks for your answer, I wonder will we use routine pool in new version server in go?

awalterschulze commented

Sorry for the delay, but I think the new server will support both some kind of worker pool or Go routine per request, depending on how the server is setup. At least that is how I am understanding it at the moment.