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

Add ReadCloser method to jwriter and buffer #77

Merged
merged 1 commit into from
Feb 17, 2017

Conversation

erikdubbelboer
Copy link
Contributor

@erikdubbelboer erikdubbelboer commented Nov 29, 2016

When marshalling something as body for a http.Request we can only use
BuildBytes() which always allocates a buffer for our whole body. This
pull request adds a ReadCloser() method which returns an io.ReadCloser
that can be passed as body into http.NewRequest. This ReadCloser will
read from the existing buffs and putBuf them when they are not needed
anymore.

@erikdubbelboer
Copy link
Contributor Author

@vstarodub and input on this?

We have been using this in productions for a while now so we know the code is stable and works.

@erikdubbelboer
Copy link
Contributor Author

@vstarodub need any help maintaining this repo?

@erikdubbelboer
Copy link
Contributor Author

Is this repo still maintained?

@erikdubbelboer
Copy link
Contributor Author

@vstarodub any update on if this will ever get merged? If not, what is the reason?

buffer/pool.go Outdated

// ReadCloser creates an io.ReadCloser with all the contents of the buffer.
func (b *Buffer) ReadCloser() io.ReadCloser {
println(len(b.bufs) + 1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove debug logging please

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ouch that's stupid :(

buffer/pool.go Outdated
}
}
// No buffers left or nothing read?
if len(r.bufs) == 0 || n == 0 {
Copy link
Contributor

@gobwas gobwas Feb 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this really be io.EOF when caller passed p with len(p) == 0?

Implementations of Read are discouraged from returning a zero byte count with a nil error, except when len(p) == 0. Callers should treat a return of 0 and nil as indicating that nothing happened; in particular it does not indicate EOF.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, net.Buffers from go 1.8 behaviour in Read() method: https://beta.golang.org/src/net/net.go?#L672

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, if you don't return io.EOF most readers will never stop waiting for additional data. The net.Buffers example you link to returns io.EOF as well. Functions like bytes.Buffer.ReadFrom only break on io.EOF.

Copy link
Contributor

@gobwas gobwas Feb 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right. But I mean the second check in that if:
Is n == 0 really the same as len(r.bufs) == 0?
I mean, what will change, if we remove the check of n == 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I guess it only makes sense when there is an empty buffer in r.bufs, which will never happen.

When marshalling something as body for a http.Request we can only use
BuildBytes() which always allocates a buffer for our whole body. This
pull request adds a ReadCloser() method which returns an io.ReadCloser
that can be passed as body into http.NewRequest. This ReadCloser will
read from the existing buffs and putBuf them when they are not needed
anymore.
@gobwas
Copy link
Contributor

gobwas commented Feb 17, 2017

Thanks! 🍻

@erikdubbelboer
Copy link
Contributor Author

Thank you.

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

Successfully merging this pull request may close these issues.

None yet

3 participants