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

Use native Okio copy for retrying. #1224

Merged
merged 1 commit into from
Jan 3, 2015
Merged

Use native Okio copy for retrying. #1224

merged 1 commit into from
Jan 3, 2015

Conversation

JakeWharton
Copy link
Member

For your consideration and review, because this has a slight behavioral problem...

This method is called from two places in two different ways:

  • SpdyTransport.writeRequestBody calls it with an instance of SpdyDataSink. This class is a Sink implementation which adds framing around a SPDY FrameWriter who ultimately delegates to a BufferedSink around the Socket.
  • HttpConnection.writeRequestBody on the other hand passes a BufferedSink which is directly wrapped around the Socket.

The distinction of the former being a Sink and the latter being a BufferedSink means that Okio.buffer is going to behave differently. When we ultimately call .emit() to ensure we're not leaking segments, one writes directly to a Socket whereas the other hands off to another Buffer and lets emission to the socket happen at the discretion of that sink.

So, are we fine with that behavior? Do we revert Okio's behavior?

@JakeWharton
Copy link
Member Author

One way to work around this behavior would be to do a single-Buffer write.

Buffer buffer = new Buffer();
thing.copyTo(buffer);
sink.write(buffer, buffer.size());

@swankjesse
Copy link
Member

I'd still like to revert Okio's behavior.

@swankjesse
Copy link
Member

... but overall, I want Okio users to not have to think about where the buffer bytes are being stranded. Whatever we do should focus on that.

@JakeWharton
Copy link
Member Author

We should do that ASAP then.

@JakeWharton JakeWharton force-pushed the jw/copy branch 2 times, most recently from 76d1598 to 595d369 Compare January 2, 2015 22:17
@JakeWharton
Copy link
Member Author

LGTY now that we're Okio 1.2.0-ing all up in this piece?

// Clone the content; otherwise we won't have data to retry.
Buffer buffer = content.clone();
socketOut.write(buffer, buffer.size());
BufferedSink bufferedOut = Okio.buffer(socketOut);
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the BufferedSource here helps?

Buffer buffer = new Buffer();
content.copyTo(buffer, 0, content.size());
socketOut.write(buffer, buffer.size());

@swankjesse
Copy link
Member

I don't really get what the motivation is here. Is clone() slower than copyTo or something?

@JakeWharton
Copy link
Member Author

I asked you that very same question a few weeks ago but I think our conversation got derailed.

@swankjesse
Copy link
Member

Cool. As it stands now, I don't think this changes behavior. LGTM.

JakeWharton added a commit that referenced this pull request Jan 3, 2015
Use native Okio copy for retrying.
@JakeWharton JakeWharton merged commit 5123b3d into master Jan 3, 2015
@JakeWharton JakeWharton deleted the jw/copy branch January 3, 2015 02:49
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.

2 participants