Skip to content
This repository has been archived by the owner on Aug 29, 2024. It is now read-only.

Better compatibility with Ruby IO. #24

Merged
merged 4 commits into from
May 3, 2019
Merged

Better compatibility with Ruby IO. #24

merged 4 commits into from
May 3, 2019

Conversation

ioquatix
Copy link
Member

@ioquatix ioquatix commented Apr 4, 2019

Fixes #23

@coveralls
Copy link

coveralls commented Apr 4, 2019

Coverage Status

Coverage decreased (-0.3%) to 90.079% when pulling ded8a89 on blocking-read-write into d606979 on master.

Copy link
Contributor

@olleolleolle olleolleolle left a comment

Choose a reason for hiding this comment

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

I read the PR and the long comment thread linked to this. Super clear.

A helpful thing in the write implementations is the “fast path” and “slow path” code comments. Keep them!

@@ -23,6 +23,10 @@

module Async
module IO
# The default block size for IO buffers.
# BLOCK_SIZE = ENV.fetch('BLOCK_SIZE', 1024*16).to_i
Copy link
Contributor

Choose a reason for hiding this comment

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

(Is this commented-out code line meant to be there?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I sometimes use it for benchmarking different block sizes :p But I don't think it's suitable for release since it's a bit of a hack.

@ioquatix
Copy link
Member Author

ioquatix commented Apr 4, 2019

@olleolleolle thanks for your feedback.

One thing I think I've grown to accept, is that async-io is a huge hack to make native ruby IO non-blocking. Sometimes it feels like whack-a-mole, and other times I wonder if I'm really appreciating how complex the edge cases can be, both POSIX, and Ruby wrapped on top.

The more I work on this code, the more I realise how difficult it is to get the right semantics without breaking the code somewhere else.

I really believe that ruby/ruby#1870 is the right solution.

@judofyr
Copy link

judofyr commented Apr 10, 2019

I haven't had time to look at this because, as you said, there's so many edge cases to take into account. That PR to Ruby looks superinteresting. I hope it gets accepted!

@ioquatix
Copy link
Member Author

@judofyr what I can say, is that an Async::IO::Generic wrapper within an Async::IO::Stream will always work predictably. So, I'd go with that for now. However, I am interested to merge this PR... I just don't know when it will happen.

@ioquatix ioquatix merged commit ed20d8b into master May 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Losing data when passing data through pipes
4 participants