Skip to content
This repository has been archived by the owner on May 4, 2018. It is now read-only.

RFC: More flexible pipe API #451

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

RFC: More flexible pipe API #451

wants to merge 1 commit into from

Conversation

Keno
Copy link
Contributor

@Keno Keno commented Jun 8, 2012

@piscisaureus As discussed on IRC and outlined in https://gist.github.com/2879135 and https://gist.github.com/2879181 this makes the pipe API more flexible by giving explicit control over both pipe ends.
I did the windows side of this first as I figured that would be the more challenging of the two implementations and would thus reveal any issues.

@Keno
Copy link
Contributor Author

Keno commented Jun 8, 2012

Also, I'm getting 4 test failures on both this branch and master: https://gist.github.com/2893501. I investigated the first one (because of the "pipe" in the name I though I had broken it), but interestingly, the failure does not occur when a the debugger is attached which makes me think it might be something race-y.

@piscisaureus
Copy link

@loladiro Did you figure out what to do about the issue that you had with ipc pipes the other day?

@Keno
Copy link
Contributor Author

Keno commented Jun 8, 2012

Yes, I don't really like it, but I made it the other pipe end include a pointer to the pid field in the parent pipe:

+  union {                  \
+    int pid;                \
+    int *p_pid;                \
+  } ipc_pid;                \

If you have a better idea, I'm all ears.

@piscisaureus
Copy link

Well, I think the that uv_read_start should just be modified to write the current process pid to the pipe, and (if that didn't happen yet), read the remote's pid from the pipe.

@Keno Keno mentioned this pull request Nov 17, 2012
@piscisaureus
Copy link

Bump @piscisaureus

@vtjnash
Copy link
Contributor

vtjnash commented Dec 18, 2012

If it helps, I updated and rebased this pull request: https://github.com/JuliaLang/libuv/tree/upipes

@indutny
Copy link
Contributor

indutny commented Apr 4, 2014

any update?

@vtjnash
Copy link
Contributor

vtjnash commented Apr 4, 2014

JuliaLang has continued to maintain an up-to-date version of this patch. The latest is part of the https://github.com/JuliaLang/libuv/tree/julia-uv0.11.22 branch, where the relevant commits are generally the first few rebased to be first after the fork. Currently, those are:
JuliaLang/libuv@cd0c19b...839ceed

@saghul
Copy link
Contributor

saghul commented Jul 29, 2014

@trevnorris please hold reviewing this until we are done with 0.12. Also, we need to go over the design before style.

@trevnorris
Copy link
Contributor

@saghul okie dokie. deleted all my style comments to remove clutter. sorry about that :)

@piscisaureus
Copy link

@Keno @vtjnash

I believe that if we added UV_INHERIT_OS_HANDLE to [this list[(https://github.com/libuv/libuv/blob/master/include/uv.h#L788), julia could create it's own pipes with pipe(2) or CreatePipe(). You wouldn't need uv_pipe_close_sync(); instead you could close the pipe with the platform-specific close() or CloseHandle().

I agree that it's not the nicest of solutions. Hopefully when we take another fresh look at the child process API we can structure things differently so this wouldn't be so much of a problem.

For now, would this work?

@vtjnash
Copy link
Contributor

vtjnash commented Nov 26, 2014

that does seem fairly reasonable. if libuv were to also provide uv_pipe_open_from_os_handle and uv_pipe2 functions, I think that would work fairly well.

@tkelman
Copy link
Contributor

tkelman commented Nov 26, 2014

@vtjnash do you think we should work on adding that as the first part of a rebase (more like partial do-over) against either master or the 1.x branch? Depending on whether we can get to it before libuv devs could.

@vtjnash
Copy link
Contributor

vtjnash commented Nov 27, 2014

reviewing the current julia-libuv diff list, it seems quite reasonable to take this approach, it just needs someone to take the time to implement a prototype/demonstration

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants