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

go lib bugs for concurrent server #570

Open
lee-qiu opened this issue Aug 24, 2023 · 4 comments
Open

go lib bugs for concurrent server #570

lee-qiu opened this issue Aug 24, 2023 · 4 comments

Comments

@lee-qiu
Copy link

lee-qiu commented Aug 24, 2023

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
Copy link
Author

lee-qiu commented Aug 24, 2023

@yfeldblum could you please take a look at this?

@awalterschulze
Copy link

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.

@lee-qiu
Copy link
Author

lee-qiu commented Sep 4, 2023

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

@awalterschulze
Copy link

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.

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

No branches or pull requests

2 participants