Skip to content

make wait_readable and wait_writable public#8651

Merged
RX14 merged 1 commit intocrystal-lang:masterfrom
stakach:patch-2
Jan 6, 2020
Merged

make wait_readable and wait_writable public#8651
RX14 merged 1 commit intocrystal-lang:masterfrom
stakach:patch-2

Conversation

@stakach
Copy link
Contributor

@stakach stakach commented Jan 5, 2020

Copy link
Member

@RX14 RX14 left a comment

Choose a reason for hiding this comment

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

reluctantly approved

@stakach
Copy link
Contributor Author

stakach commented Jan 6, 2020

Thanks @RX14
I feel that access to functions like this are what make a language powerful.

Agree that they shouldn't be used day to day but useful when doing something a little more complex that lies outside of the typical.

I'd put this in a similar category to the uninitialized keyword

@straight-shoota
Copy link
Member

@ysbaddaden Is there any particular reason this was reverted in #7505?

@ysbaddaden
Copy link
Collaborator

Because it's internal API detail, and there aren't any need nor incentive for them to be publicly available, and nobody complained in over a year. Be aware that using this will break, most likely when adding support for Windows, or moving from libevent to a custom solution.

I'd love to see a real use-case. If it's just a "feel that it should be accessible", then let's close this PR.

@RX14
Copy link
Member

RX14 commented Jan 6, 2020

@ysbaddaden the real usecase is binding C libraries: https://github.com/spider-gazelle/ssh2.cr/blob/a93eb957420b9e9f31bb8cd46e50ff862c5bc54b/src/session.cr#L39-L49

libgit2 returns EAGAIN from function calls when it needs to read more data, just like read and write, then expects the caller to handle them like read and write. So it needs this API.

I suspect when we abstract the event loop, we will have Crystal::System::EventLoop.read(fd) which will call Crystal::System::EventLoop.wait_readable(fd) as part of it's implementation. The latter will not exist on Windows. We could make the latter public (though undocumented and unstable) instead of private to support this kind of implementation on unix at least.

Or, we could force users to use threads, which i'm not super against anyway.

@ysbaddaden
Copy link
Collaborator

@RX14 ok.

@RX14 RX14 added this to the 0.33.0 milestone Jan 6, 2020
@RX14 RX14 merged commit e4ad1de into crystal-lang:master Jan 6, 2020
@bcardiff
Copy link
Member

bcardiff commented Jan 6, 2020

The refactors in #7505 is where the changes in #7366 got dropped. (this is maybe a note for future self to write the changelog 🙈 )

elebow added a commit to elebow/minyaty that referenced this pull request Apr 2, 2022
We can use IO::FileDescriptor#wait_readable (an undocumented but public
method) to yield a fiber until the file descriptor has something to read.

On POSIX systems (only), the X11 socket is exposed as a file descriptor.
That's an acceptable limitation for now---I don't care to support X11 on
Windows.

crystal-lang/crystal#7366
crystal-lang/crystal#8651
https://forum.crystal-lang.org/t/fibers-blocking-io-and-c-libraries/3116/13?u=elebow
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants