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

Add support for pipe syscall (PIPEFS) #4378

Closed
wants to merge 5 commits into from

Conversation

cynecx
Copy link
Contributor

@cynecx cynecx commented Jun 1, 2016

Addressing #2883.

  • Add test for pipefs
  • Add more tests for pipefs

@kripken
Copy link
Member

kripken commented Jun 1, 2016

An overview might help here - what does this do, in the big picture?

@cynecx
Copy link
Contributor Author

cynecx commented Jun 1, 2016

This is needed to get boost/asio to work with emscripten. See issue #2883.

Every call to the pipe syscall will basically create a new fifo buffer which users can write and read to. That's actually it. Asio uses this mechanism to interrupt an event loop. Yeah, I know this sound paradox as the browser is itself event-driven, this means that the user who wants to use asio has to integrate asio's event loop into the browser's one by calling the poll_one function.

@kripken
Copy link
Member

kripken commented Jun 3, 2016

Ok, thanks.

});
wNode.stream = writableStream;

return [ readableStream.fd, writableStream.fd ];
Copy link
Member

Choose a reason for hiding this comment

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

may as well return an object here, so you don't need to use 0 and 1 to index, but can use proper names.

@kripken
Copy link
Member

kripken commented Jun 3, 2016

What's the status of a test for this?

@cynecx
Copy link
Contributor Author

cynecx commented Jun 4, 2016

Addressed all comments. A test will follow soon as I am currently heavily testing asio.

@kripken
Copy link
Member

kripken commented Jun 6, 2016

Thanks, looks good so far. Waiting on the test, as it'll help me understand this more completely.

@juj
Copy link
Collaborator

juj commented Jun 8, 2016

Will this get correctly dead code eliminated away if one doesn't call pipe() in a program at all? I.e. no syscall42? That is worth checking, since this will be somewhat rarely used functionality, so that it won't be pulled in for builds that don't need it.

@cynecx
Copy link
Contributor Author

cynecx commented Jun 9, 2016

Sorry about the delay, I was kinda busy these days. I will try to write some basic tests now.

@juj Do you mean we should put this behind an #ifdef USE_PIPE?

@kripken
Copy link
Member

kripken commented Jun 10, 2016

I think it should be ok: PIPEFS is a dependency of that syscall, and our syscalls are static, so it should only get pulled in if the syscall is actually called. We should verify to be sure, but if not, that would be a separate bug in our linking.

@juj
Copy link
Collaborator

juj commented Jun 20, 2016

Yeah, no need to have #ifdef if DCE works correctly already without it.

@seibelj
Copy link

seibelj commented Jun 21, 2016

👍 would appreciate a merge

@atrosinenko
Copy link
Contributor

Is this PR still planned for merge? It would be great to have the pipe syscall working in Emscripten for the purpose of events.

@kripken
Copy link
Member

kripken commented Jan 21, 2017

Looks like this PR is waiting on tests - we can't merge it without that. Also it needs to be rebased.

@atrosinenko
Copy link
Contributor

I am not familiar with Emscripten FS layer, but in case you consider this implementation looking good enough, then maybe I can somehow help you with unit-testing it according to the pipe syscall documentation?

@cynecx
Copy link
Contributor Author

cynecx commented Jan 25, 2017

I have rebased the branch but unfortunately I am currently unable to write tests because I simply don't have the time to setup the environment as I am very busy right now with other things.

However, it would be great if someone else could write some tests for this patch. I would really appreciate this :).

@atrosinenko
Copy link
Contributor

Thank you, @cynecx. How can I contribute tests? Should this PR be integrated first into some feature branch of kripken/emscripten repo so I can pull it and add some tests?

@kripken
Copy link
Member

kripken commented Jan 30, 2017

I think the simplest thing is for you to create a branch in your repo, then merge in @cynecx's branch, then add the tests, then make a PR with all that together.

@atrosinenko
Copy link
Contributor

This PR can probably be closed because it was integrated in #4935.

@juj
Copy link
Collaborator

juj commented Jul 21, 2017

Thanks! Closing.

@juj juj closed this Jul 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants