-
Notifications
You must be signed in to change notification settings - Fork 48
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
connection pool and some tests #6
base: master
Are you sure you want to change the base?
Conversation
…struct - made private method parse into function - exposed apis are unchanged
Hi, there is error when run test.go:
|
well, I will modify the ip and port in test.go |
Thanks, I should've switched them back. |
you must not make recv_buf a local variable in recv, it should be made as instance variable, because it may receive more than one response at a time. when recv() returns the first response, there is still other response(s) in recv_buf. |
Actually, ssdb interaction protocol is not a request-response protocol, there are commands that will recv more than one response packet. one is |
I have just updated the master, added Send/Recv methods, and showed how to use |
The big problem that needs solving is that this client does not handle parallel commands. If you put this in a multi-threaded server it will hang. When there are many requests coming in parallel, they all use the same send/recv into the same socket, and they get called at different times, unpredictably. I'll make a sample to demonstrate. Multiple connections solves it, but its not necessary to make multiple connections. If the single connection does one transaction at a time it will work. I don't mind moving the recv buffer back into the object. That's one simple revert. I made several commits so I could back some of them out. |
… Client struct - made private method parse into function - exposed apis are unchanged" This reverts commit 4e61e4b.
…lict to get back to head of master Merge branch 'master' of https://github.com/ssdb/gossdb Conflicts: ssdb/ssdb.go test.go
I put the recv_buff back into the client object. Thanks for the feedback and the sync140 example. I didn't now about that. The sync140 works here, and the tests all pass. |
Here's some code that shows the problem. It doesn't always hang, but it doesn't work right. I get unpredictable bad behaviour. If you use my code in this patch this parallel loop case works correctly. Put this into your test.go:
Run it several times. You may get some hanging, and other bad behaviour. What do you think? |
For other programming language, create new connections on demand is recommended, for golang, the user should limit the number of concurrent goroutines, letting too many goroutines running in the same time is cheap in golang, but it is not recommend by me. I would rather redesign my program's work flow to reduce on-going async procedures. So this is why connection pool support is not considered as an option in gossdb.
I would never write down code like this
except that I only just to show somebody this is totally wrong. |
I only created that example to demonstrate bad behaviour. I see similar bad behaviour when I use gossdb with a well written server. When the server gets hammered with many parallel requests, the gossdb client seems to get tangled up. I don't recommend such bad go code either. Its an example to illustrate the problem I've seen. |
The recommend way to use gossdb is to create separated connection on separated goroutine. If someone don't follow the instruction, it is his responsibility to do it in the right way. I will write this statement clear on docs: gossdb is not thread safe. |
OK, you can close this. I'll see if I can get my server to use the client as you suggest. |
This patch primarily does two things.
It doesn't alter the existing API for exposed functions.
The new multi-connection functionality has a different entry point, but the api usage is the same as the non-pooled, and internally it calls the regular non-pool functions. The pool calls have the same name and syntax as the non-pool calls.
It also provides a standard golang test suite.