-
Notifications
You must be signed in to change notification settings - Fork 478
Update ChanReader / ChanWriter for use inside toxics #134
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
base: dev
Are you sure you want to change the base?
Conversation
c82084d to
0482278
Compare
|
Went through this again and cleaned some things up now that it's not fresh in my mind. |
| } | ||
| writer.Write(buf[:n]) | ||
| stub.Writer.Write(buf[:n]) | ||
| stub.Reader.Checkpoint(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why Checkpoint(0)? Should it not be Checkpoint(-stub.Reader.Buffered())?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The example where I used .Buffered() was for a bufio.Reader attached to the TransactionalReader.
In this case, the amount of buffered data is always 0. Since everything read is being used, we can set the checkpoint to the last read byte, rather than the (last read) - (amount buffered by bufio.Reader)
|
|
||
| func NewTransactionalReader(input <-chan *StreamChunk) *TransactionalReader { | ||
| t := &TransactionalReader{ | ||
| buffer: bytes.NewBuffer(make([]byte, 0, 32*1024)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you choose 32K sized buffer for a specific reason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I chose 32K because it's the size used by io.Copy(), however bytes.Buffer will grow the size of the buffer as necessary.
This is just a nice default size so in most cases it doesn't need to be reallocated.
stream/io_chan.go
Outdated
| current = int(t.bufReader.Size()) - t.bufReader.Len() | ||
| } | ||
|
|
||
| n := current |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initializing n to current seems to be pointless here? Why not var n int?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, looks like this is redundant. I'll see if I can refactor this function and make it a little simpler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually found another bug while looking at this. if bufReader != nil, t.buffer.Reset() shouldn't be called unless the offset is big enough.
I fixed this, and added a test for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to get my head around this buffered stuff so we can get this out the door too.Do say if you want to ship this yourself @xthexder. These PR comments are for my own understanding so that I can keep reviewing later.
| Reader: stream.NewTransactionalReader(input), | ||
| Writer: stream.NewChanWriter(output), | ||
| } | ||
| stub.Reader.SetInterrupt(stub.Interrupt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this Interupt monkey business.
| current := t.buffer.Len() | ||
| if t.bufReader != nil { | ||
| current = int(t.bufReader.Size()) - t.bufReader.Len() | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So current represents how much of the buffer we've already read.
0will say... okay we've read everything! Lock it in!-5will say we've read everything except for the last 5 bytes. Let's start reading again from the start of those last 5.5will say we successfully read 5 bytes but not the rest. Let's start reading from the rest.t.buffer.Len()will do the same thing is0.
Okay I found this confusing but LGTM
| } | ||
| if len(buf[n:]) > 0 { | ||
| t.reader.Read(buf[n:]) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this correct? Why are we trying to flush the reader? It's not buffered is it? I'll look at this again later and probably understand right away.
While working on the bidirectional toxics PRs I discovered some problems with how
ChanReaderworked when used ephemerally inside a toxic.These turned out to be non-trivial to fix, with the end result being this PR.
The problem with
ChanReaderis that in some cases it may buffer data. This means if you ever stop using the reader and switch to another, there's a possibility of losing data.Further problems arise when
ChanReaderis read using other readers such asbufio.Readeras required by functions such ashttp.ReadRequest().These problems are solved in 3 ways:
ToxicStubinstead of creating it every timePipe()is called.TransactionalReaderthat can be rolled back to specific points so thatbufio.Readercan effectively be un-read.Example
TransactionalReaderusage:Actual usage can be found in my WIP redis toxic here: https://github.com/Shopify/toxiproxy/blob/redis-wip/toxics/redis.go
The documentation for this is pretty tricky to write without actual code examples, so I'll likely leave the detailed docs until we have at least 1 protocol-aware toxic to point at.
@sirupsen @jpittis / anyone else interested in Golang