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

Fix peek in libasync #1179

Merged
merged 3 commits into from Jul 16, 2015

Conversation

Projects
None yet
2 participants
@etcimon
Contributor

etcimon commented Jul 10, 2015

The peek function returned at most 1 byte due to a logical error. This also fixes an assertion failure when finalizing a read-only stream (not sure if it should fail)

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jul 10, 2015

Member

Actually it looks like the whole semantics of peek() are off. peek is supposed to return a slice of an internal buffer (can have length zero), but does not empty the buffer. Since read() is called here, I suppose that it consumes the input instead, right?

Member

s-ludwig commented Jul 10, 2015

Actually it looks like the whole semantics of peek() are off. peek is supposed to return a slice of an internal buffer (can have length zero), but does not empty the buffer. Since read() is called here, I suppose that it consumes the input instead, right?

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Jul 10, 2015

Contributor

Since read() is called here, I suppose that it consumes the input instead, right?

Yes, but internally the calls to libasync are atomic, so as long as the m_offset returns to the same spot it shouldn't produce any unwanted side effects. At least, that's worked for my tests on image uploads using the multipart-upload branch.

Contributor

etcimon commented Jul 10, 2015

Since read() is called here, I suppose that it consumes the input instead, right?

Yes, but internally the calls to libasync are atomic, so as long as the m_offset returns to the same spot it shouldn't produce any unwanted side effects. At least, that's worked for my tests on image uploads using the multipart-upload branch.

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Jul 10, 2015

Contributor

I used the peek to capture the post data for the debugger. Check this out:

https://gist.github.com/etcimon/fa075692ff9ab91e739d

It captures only 1 request that matches certain criterias, using this form:
http://htmlpreview.github.io/?https://github.com/etcimon/vibe.d/blob/master/views/capture.html

It works in --build=release and --build=plain, so my compile time is pretty quick =)

Contributor

etcimon commented Jul 10, 2015

I used the peek to capture the post data for the debugger. Check this out:

https://gist.github.com/etcimon/fa075692ff9ab91e739d

It captures only 1 request that matches certain criterias, using this form:
http://htmlpreview.github.io/?https://github.com/etcimon/vibe.d/blob/master/views/capture.html

It works in --build=release and --build=plain, so my compile time is pretty quick =)

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jul 10, 2015

Member

Hm, okay, I missed the m_offset -=... but then there is another issue: leastSize has to block until either data is available, or until the connection is closed. peek on the other hand must never block and returns an empty slice instead.

So actually leastSize is the one that doesn't have the right semantics.

Member

s-ludwig commented Jul 10, 2015

Hm, okay, I missed the m_offset -=... but then there is another issue: leastSize has to block until either data is available, or until the connection is closed. peek on the other hand must never block and returns an empty slice instead.

So actually leastSize is the one that doesn't have the right semantics.

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Jul 10, 2015

Contributor

So actually leastSize is the one that doesn't have the right semantics.

Yes so that would be determined in the constructor, I think the right thing to do is use the local parameters for this

Contributor

etcimon commented Jul 10, 2015

So actually leastSize is the one that doesn't have the right semantics.

Yes so that would be determined in the constructor, I think the right thing to do is use the local parameters for this

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jul 10, 2015

Member

Ohhhh, forget what I said :) This is of course irrelevant, because we are talking about the file stream and not a network stream. In this case, peek should simply always return null (unless there actually is an internal buffer). The key is that peek may never block the event loop, but it also shouldn't allocate for performance reasons.

Member

s-ludwig commented Jul 10, 2015

Ohhhh, forget what I said :) This is of course irrelevant, because we are talking about the file stream and not a network stream. In this case, peek should simply always return null (unless there actually is an internal buffer). The key is that peek may never block the event loop, but it also shouldn't allocate for performance reasons.

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Jul 10, 2015

Contributor

This is of course irrelevant, because we are talking about the file stream and not a network stream. In this case, peek should simply always return null (unless there actually is an internal buffer)

Oh, right, wouldn't this break readUntil though?

Contributor

etcimon commented Jul 10, 2015

This is of course irrelevant, because we are talking about the file stream and not a network stream. In this case, peek should simply always return null (unless there actually is an internal buffer)

Oh, right, wouldn't this break readUntil though?

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Jul 10, 2015

Contributor

Nevermind, it doesn't. Alright then, I'll return null and use a different strategy to actually peek that data :-p

Contributor

etcimon commented Jul 10, 2015

Nevermind, it doesn't. Alright then, I'll return null and use a different strategy to actually peek that data :-p

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jul 10, 2015

Member

Oh, right, wouldn't this break readUntil though?

readUntil only uses peek for optimization purposes. It will issue a series of little read calls instead if peek returns null. A worthwhile optimization could be to instead read larger chunks if the stream is a RandomAccessStream.

Member

s-ludwig commented Jul 10, 2015

Oh, right, wouldn't this break readUntil though?

readUntil only uses peek for optimization purposes. It will issue a series of little read calls instead if peek returns null. A worthwhile optimization could be to instead read larger chunks if the stream is a RandomAccessStream.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jul 10, 2015

Member

Anyway, I'll have to explicitly mention this part of peek's behavior in the documentation.

Member

s-ludwig commented Jul 10, 2015

Anyway, I'll have to explicitly mention this part of peek's behavior in the documentation.

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Jul 10, 2015

Contributor

Hm, yes I'll readAll and seek(0) to get the behavior I'm looking for.

Contributor

etcimon commented Jul 10, 2015

Hm, yes I'll readAll and seek(0) to get the behavior I'm looking for.

s-ludwig added a commit that referenced this pull request Jul 16, 2015

@s-ludwig s-ludwig merged commit 5f851c7 into vibe-d:master Jul 16, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment