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

Fix losing data when reading from STDIN. #2872

Merged
merged 1 commit into from
Sep 4, 2018

Conversation

sgebbie
Copy link
Contributor

@sgebbie sgebbie commented Sep 3, 2018

The current implementation fills the full underlying array based on the
"space" available.

However, the space may be larger than the chunk size since space is
allocated in powers of two. However, when the data buffer is passed back
via the InputNotify handle it is first appropriately truncated to the
actual len read by @pony_os_stdin_read. But, the implementation of
truncate sets the size to max(len, size) and does not take into account
the underlying allocated space.

The net result is that reading of stdin looses data unless the chunk
size provided is actually a power of two — in which case
_size == _alloc.

This change simply limits the read to data.size() rather than
data.space().

Please see: #2871.

The current implementation fills the full underlying array based on the
"space" available.

However, the space may be larger than the chunk size since space is
allocated in powers of two. However, when the data buffer is passed back
via the InputNotify handle it is first appropriately truncated to the
actual `len` read by @pony_os_stdin_read. But, the implementation of
truncate sets the size to max(len, size) and does not take into account
the underlying allocated space.

The net result is that reading of stdin looses data unless the chunk
size provided is actually a power of two — in which case
`_size == _alloc`.

This change simply limits the read to `data.size()` rather than
`data.space()`.
Copy link
Contributor

@mfelsche mfelsche left a comment

Choose a reason for hiding this comment

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

Thank you! Good catch!
That makes very much sense.

Nonetheless I do have a bad feeling that we don't have no tests for this runtime behaviour.

We should at some point include running compiled pony programs and assert on their behaviour as a kind of integration test. But this is a story for another time.

@mfelsche mfelsche added the changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge label Sep 4, 2018
@mfelsche mfelsche changed the title Change stdin read to honour chunk size. Fix losing datra when reading from STDIN. Sep 4, 2018
@mfelsche mfelsche changed the title Fix losing datra when reading from STDIN. Fix losing data when reading from STDIN. Sep 4, 2018
@mfelsche mfelsche merged commit ea9a478 into ponylang:master Sep 4, 2018
ponylang-main added a commit that referenced this pull request Sep 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants