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

Refactor IO::Syscall as IO::Evented #7505

Merged

Conversation

Projects
None yet
7 participants
@ysbaddaden
Copy link
Member

commented Mar 4, 2019

I started this refactor in my MT experiments, but I think it stands on its own:

Evented is closer to what the module is trying to achieve (abstract event loop details), and method naming follows that from IO::Buffered (e.g. evented_read instead of read_syscall_helper).

Refactors the FileDescriptor and Socket IO classes to move implementation details into IO::Evented to 1. reduce duplication, and 2. avoid direct accesses to internal details (such as @writers or @read_event).

Refactor IO::Syscall as IO::Evented
Evented is closer to what the module is trying to achieve (abstract
event loop details) than Syscall. Reuses the method namings from
IO::Buffered (e.g. `evented_read` instead of `read_syscall_helper`).

Avoids some duplication found in IO::FileDescriptor (system/unix)
and Socket.

Isolates implementation details back into IO::Evented, instead of
having direct accesses to internal implementation details (such as
`@WriterS`) of IO::Evented.
@straight-shoota
Copy link
Member

left a comment

While you're at it, could you add a short documentation to IO::Evented, and maybe some of its methods?

@ysbaddaden

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2019

Huum, I think it should be :nodoc:. Maybe moved to Crystal namespace, along Event and EventLoop. Maybe even moved to Crystal::System since it's only used on POSIX platforms.

@yxhuvud

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

(quite OT, but related to the greater topic of evented IO on Linux) I wonder how much of this will change when https://lwn.net/Articles/776703/ is fully merged and accessible from userspace.

@RX14

RX14 approved these changes Mar 4, 2019

Copy link
Member

left a comment

I think I originally designed it expecting to be able to adapt the class for windows, so I put the events outside the mixin. I don't think I will be able to reuse this so eventually I think IO::Evented will move under Crystal::System and become UNIX-only.

That's just to say expect future refactors: I won't know for sure until I try and get windows networking code working. This is fine to merge as-is.

@ysbaddaden ysbaddaden requested a review from bcardiff Mar 7, 2019

@ysbaddaden

This comment has been minimized.

Copy link
Member Author

commented Mar 7, 2019

Having them moved back into IO::Evented helped in my MT experiments, thought I moved most libevent responsibility into EventLoop, and use a single Event on Fiber directly, which holds the status (e.g. added, canceled). See ysbaddaden@2ea421a

@ysbaddaden

This comment has been minimized.

Copy link
Member Author

commented Mar 14, 2019

@bcardiff it was already :)

@bcardiff
Copy link
Member

left a comment

I agree with @straight-shoota that would be great to have some documentation, even if it's moved to crystal namespace. But that can come later. It's a great refactor.

@bew

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

It looks like everybody agree, let's get going! 🚀
ok

@straight-shoota straight-shoota merged commit f9fde1a into crystal-lang:master Mar 15, 2019

5 checks passed

ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@straight-shoota straight-shoota added this to the 0.28.0 milestone Mar 15, 2019

@straight-shoota

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

Thank you @ysbaddaden!

@ysbaddaden ysbaddaden deleted the ysbaddaden:refactor/io-syscall-to-evented branch Mar 15, 2019

urde-graven pushed a commit to urde-graven/crystal that referenced this pull request Mar 20, 2019

Urde Graven
Merge branch 'master' of github.com:crystal-lang/crystal
* 'master' of github.com:crystal-lang/crystal:
  Change the font-weight used in Playground (crystal-lang#7552)
  Fix formatting absolute paths (crystal-lang#7560)
  Refactor IO::Syscall as IO::Evented (crystal-lang#7505)
  YAML: fix test checking serialization of slices for libyaml 0.2.2 (crystal-lang#7555)
  Let Array#sort only use `<=>`, and let `<=>` return `nil` for partial comparability (crystal-lang#6611)
  Update `to_s` and `inspect` to have similar signatures accross the stdlib (crystal-lang#7528)
  Fix restriction of valid type vs free vars (crystal-lang#7536)
  Rewrite macro spec without executing a shell command (crystal-lang#6962)
  Suggest `next` when trying to break from captured block  (crystal-lang#7406)
  Fix GenericClassType vs GenericClassInstanceType restrictions (crystal-lang#7537)
  Add human readable formatting for numbers (crystal-lang#6314)
  Add command and args to execvp error message (crystal-lang#7511)
  Implement Set#add? method (crystal-lang#7495)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.