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

feat: wrap GZIPInputStream for connection reuse #840

Merged
merged 3 commits into from Oct 18, 2019

Conversation

@codyoss
Copy link
Member

codyoss commented Oct 14, 2019

If a connection is closed and there are some bytes that have not
been read that connection can't be reused. Now GZIPInputStream
will have all of its bytes read on close automatically to promote
connection reuse.

Cherry-picked: #749
Fixes: #367

chingor13 and others added 2 commits Jul 15, 2019
If a connection is closed and there are some bytes that have not
been read that connection can't be reused. Now GZIPInputStream
will have all of its bytes read on close automatically to promote
connection reuse.

Cherry-picked: #749
Fixes: #367
@codyoss codyoss requested a review from googleapis/yoshi-java as a code owner Oct 14, 2019
@googlebot googlebot added the cla: yes label Oct 14, 2019
@codyoss

This comment has been minimized.

Copy link
Member Author

codyoss commented Oct 14, 2019

Noticed that #749 had not been updated in a while so I cherry picked and fixed the PR comments.


/**
* This class in meant to wrap an {@link InputStream} so that all bytes in the steam are read and
* discarded on {@link InputStream#close()}. This ensures that the underlying connection has the

This comment has been minimized.

Copy link
@elharo

elharo Oct 14, 2019

Collaborator

What if the stream never ends?

This comment has been minimized.

Copy link
@codyoss

codyoss Oct 14, 2019

Author Member

That is a very good question. I am not sure.

public void close() throws IOException {
if (!closed && inputStream != null) {
try {
ByteStreams.exhaust(this);

This comment has been minimized.

Copy link
@elharo

elharo Oct 14, 2019

Collaborator

I wonder if we could have an infinite loop here. See internal discussion.

@codyoss

This comment has been minimized.

Copy link
Member Author

codyoss commented Oct 17, 2019

@elharo I fixed all of the comments. Do you still have reservations around a possible infinite stream? If so, any thought on how we should move forward with this?(Maybe we just don't if we think the infinite stream is too big of a risk as this feature requires exhausting all of the extra bytes for it to work.)

@elharo
elharo approved these changes Oct 18, 2019
Copy link
Collaborator

elharo left a comment

given how this is used and that it's non-public the infinite steam issue probably won't come up

@codyoss codyoss merged commit 087a428 into googleapis:master Oct 18, 2019
7 checks passed
7 checks passed
Kokoro - Test: Dependencies Build successful
Details
Kokoro - Test: Java 11 Build successful
Details
Kokoro - Test: Java 7 Build successful
Details
Kokoro - Test: Java 8 Build successful
Details
Kokoro - Test: Linkage Monitor Build successful
Details
Kokoro - Test: Windows Java 8 Build successful
Details
cla/google All necessary CLAs are signed
@codyoss codyoss deleted the codyoss:chuncked-keepalive branch Oct 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.