I’ve casually been working on something that solved a need I had in a few projects, and have been trying to get some feedback on it to see if it’s anything people see value in. I’ve posted it before for people who were looking into using TCP to communicate between processes in the gopher slack, so me dropping this link and asking for feedback might be a bit of a broken record at this point
The high level story: In two different projects I wrote code to handle streaming protobuffs over TCP, and decided to rip it all out and make a single library out of it. I tried as hard as I could to make it look as much like something in the net package as possible, but I chose to not use the net.Conn interface for possible performance reasons (I set a goal to hit 1mil+ messages per second and did whatever I could to stay north of that - for better or worse.). I got it to about 1.1mil / sec between two EC2 nodes with no errors (saturating a 1gig nic) on a single listening socket, and put a bow on it for now.
The idea behind it is that you’d build systems on top of this- it provides no proper way to handle a message that completely failed to send, nor one that completely fails to be processed by a server - but it does give the minimum amount of information / access necessary to have client code do that work (so I believe, anyways). I wanted something that wrapped all the edge-casey boilerplate of writing a format like protocolbuffers over TCP (including padding/stripping the size from the messages, and handling TCP back-pressure cases where writes begin to partially complete), so that people had a very simple Writer and Reader struct that handled 95% of their needs for them (except for the aforementioned absolute failure cases).
Any feedback at all is sought - even / especially “This is a terrible idea, would never use it in a million years”. At the end of the day, it was a great learning opportunity about writing tcp code in golang, so even if it’s terrible, it was still a good use of time (imo, anyways lol).
First of all, I think this is a good idea and I would use it. I’ve written the header code a few times myself, so I’d probably use it for that alone.
Since I didn’t have time or brain today to look at the code I’ll give some suggestions from a library maintenance & usability perspective:
Provide some examples in the Godocs. At least an example server & client would be really helpful for people to see why and how to use buffstreams.
Any reason why the bytes get sent to a callback instead of streamed through a channel? Maybe I’m misunderstanding the purpose, but seems like a channel is better suited for streaming.
I think more documentation on how BuffManager works is necessary. Here are 3 questions that were hard to answer:
What does it do with idle connections?
Does it recycle connections, and if so, how?
Can I cancel in-flight streams?
This one is about naming. People will use the library as buffstreams.YourThing, so don’t prefix stuff with Buff. buffstreams.Manager instead of buffstreams.BuffManager for example
Overall, this looks like great work. Thanks for sharing.
There actually is some sample code, not in the godoc, but in the Readme on github, as well as in the test/ directory with a sample client / server and message. That was sort of the most basic level example I could think to provide, but perhaps I could call that out better, and maybe add to the godoc. Same goes for the BuffManager notes. I could do a much better job explaining how it works and what it does.
As for not using channels, I’ve read a bit and seen some benchmarks stating that they are not great for code that is sensitive to performance - and I wanted to (perhaps to the detriment of the API) do whatever I could to avoid anything that might be slow at the outset. I do have a note in the roadmap to look at providing a channel option, so that’s something I’m already thinking about - but I wanted to make it so you could use either a callback or a channel, so I’m thinking through that design a bit before committing to anything.
As for naming… I think the entire project has a stupid name :). I could probably remove some of the “Buff” prefixes, since as you point out, they’re redundant. I’m hesitant to make big changes though, due to the lack of a proper way to pin and manage library dependencies outside of one of a few different vendoring solutions… but then again, no one is probably using this so I’m pretty clear to do as I please lol.
Is the library widely used in the wild already ? If not, I think it’s reasonable to still make breaking changes until you formally announce the lib to be “stable”
I have announced it on twitter, etc etc. I’m actually giving a talk in a few days on some of the stuff I learned about writing TCP code in go, where I’ll finish by talking about the library. But, I doubt it’s really being used,or else I’d imagine i’d see issues or PRs coming in about certain things.
So, I probably do have the leeway to make breaking changes.
Just wanted to drop a note saying that I took the feedback to heart, and have made a set of changes that removes all the “Buff” prefixing from things, fixes some bugs (doh!), and still meets the benchmarks currently posted (and now officially has been tested under 1.5, and have expanded my travis cli to include all of 1.4 and 1.5). I also updated a few parts of the read me to better call out the Manager stuff, and added a note about the test server as an end to end example.
Would still love any other feedback anyone is interested in providing.
