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

Added initial implementations of stdin InputStream and stdout/stderr OutputStreams #729

Merged
merged 2 commits into from Jul 18, 2014

Conversation

Projects
None yet
2 participants
@EricMCornelius

EricMCornelius commented Jul 16, 2014

Let me know if this seems worthwhile - I tend to use similar functionality for node.js pipelines from stdin and to stdout/stderr fairly often.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jul 16, 2014

Member

I have uploaded a gist that implements wrapper for std.stdio streams using threads, so that they don't block the event loop. This still isn't perfect, but using standard blocking I/O directly, depending on how the data gets send, can lead to very unexpected hangups, so that I'd not want to include it like that in the library.

But probably replacing StdOutStreamImpl/StdInStreamImpl with the implementation in that gist would be good enough for inclusion. I just didn't do it up to now, because I didn't think enough about the possible future implications.

Member

s-ludwig commented Jul 16, 2014

I have uploaded a gist that implements wrapper for std.stdio streams using threads, so that they don't block the event loop. This still isn't perfect, but using standard blocking I/O directly, depending on how the data gets send, can lead to very unexpected hangups, so that I'd not want to include it like that in the library.

But probably replacing StdOutStreamImpl/StdInStreamImpl with the implementation in that gist would be good enough for inclusion. I just didn't do it up to now, because I didn't think enough about the possible future implications.

@EricMCornelius

This comment has been minimized.

Show comment
Hide comment
@EricMCornelius

EricMCornelius Jul 16, 2014

Makes sense, I'll look at getting that added. Also, is there a compiled style guide you'd like me to follow as I make contributions?

EricMCornelius commented Jul 16, 2014

Makes sense, I'll look at getting that added. Also, is there a compiled style guide you'd like me to follow as I make contributions?

@EricMCornelius

This comment has been minimized.

Show comment
Hide comment
@EricMCornelius

EricMCornelius Jul 16, 2014

Taking a look at integrating the gist - I get the following when running directly in main:

core.exception.AssertError@../../vibe.d/source/vibe/core/drivers/libevent2.d(656): ManualEvent.wait works only when called from a task.

I'm still familiarizing myself with the concurrency model, but is there a mechanism to avoid needing to run it inside a task?

void pipeInOut() {
  auto input = new StdinStream();
  auto output = new StdoutStream();
  output.write(input);
}

shared static this()
{
  // this works
  // runTask(toDelegate(&pipeInOut));

  // this doesn't
  pipeInOut();
}

EricMCornelius commented Jul 16, 2014

Taking a look at integrating the gist - I get the following when running directly in main:

core.exception.AssertError@../../vibe.d/source/vibe/core/drivers/libevent2.d(656): ManualEvent.wait works only when called from a task.

I'm still familiarizing myself with the concurrency model, but is there a mechanism to avoid needing to run it inside a task?

void pipeInOut() {
  auto input = new StdinStream();
  auto output = new StdoutStream();
  output.write(input);
}

shared static this()
{
  // this works
  // runTask(toDelegate(&pipeInOut));

  // this doesn't
  pipeInOut();
}
@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jul 17, 2014

Member

ManualEvent.wait() should work now outside of a task. Can you test if that fixed it? I just tested it in a very simple situation so far.

Member

s-ludwig commented Jul 17, 2014

ManualEvent.wait() should work now outside of a task. Can you test if that fixed it? I just tested it in a very simple situation so far.

@EricMCornelius

This comment has been minimized.

Show comment
Hide comment
@EricMCornelius

EricMCornelius Jul 17, 2014

Looks good in my local testing

EricMCornelius commented Jul 17, 2014

Looks good in my local testing

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jul 18, 2014

Member

Okay, thanks! Merging in.

PS: A style guide is located on vibed.org: http://vibed.org/style-guide

Member

s-ludwig commented Jul 18, 2014

Okay, thanks! Merging in.

PS: A style guide is located on vibed.org: http://vibed.org/style-guide

s-ludwig added a commit that referenced this pull request Jul 18, 2014

Merge pull request #729 from EricMCornelius/stdio_streams
Added initial implementations of stdin InputStream and stdout/stderr OutputStreams

@s-ludwig s-ludwig merged commit de7d290 into vibe-d:master Jul 18, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@EricMCornelius EricMCornelius deleted the EricMCornelius:stdio_streams branch Jul 18, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment