You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Cliff Jansen <cl...@gmail.com> on 2014/03/07 06:48:37 UTC
Review Request 18893: Extending PROTON-525 to Windows platforms (selectors)
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18893/
-----------------------------------------------------------
Review request for qpid and Rafael Schloming.
Bugs: PROTON-529
https://issues.apache.org/jira/browse/PROTON-529
Repository: qpid
Description
-------
This patch is a minimal port of the existing Windows code base to incorporate the refactored IO and selector mechanism of PROTON-525. It does not incorporate any changes to how the sockets and "pipes" operate but some thought was given to accommodating completion ports right after 0.7.
Although there was a clear attempt in PROTON-525 to provide rich abstractions that would allow Posix and Windows implementations to optimize their low level IO for each OS, awkwardness remains.
Windows sockets are outside the regular IO system, for a long time a bolt on third party implementation. For Posix, they are an extension of regular file descriptors and things like close(socket_fd) makes sense.
This short term Windows fix hides the above. Because the "sockets only" select() call in the Windows api was the easiest to work with initially, it was necessary to fake a socket-based pipe for the control port. Since everything is sockets anyway, we can mix the control fd with other fds. If we used Windows pipes, we couldn't. But we definitely don't want to use select() long term and we may not want to use fake socket pipes, or even any pipes at all.
Note that the pipe control mechanism is not needed on Windows if you use completion ports and is not used in the Qpid C++ Windows implementation.
Even though it is probable that the existing new api could be made to work without penalizing Windows significantly, I would prefer to stop mingling sockets with non socket io objects in proton.
Perhaps some interrupt interface could be added to the selector, implemented with pipes on (some) Posix systems and optimized in other ways on other platforms.
Diffs
-----
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/CMakeLists.txt 1573021
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/include/proton/cproton.i 1573021
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/include/proton/io.h 1573021
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/include/proton/selectable.h 1573021
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/messenger/messenger.c 1573021
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/posix/driver.c 1573021
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/posix/io.c 1573021
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/selectable.h 1573021
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/selectable.c 1573021
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/ssl/ssl-internal.h 1573021
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/ssl/ssl_stub.c 1573021
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/windows/driver.c 1573021
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/windows/io.c PRE-CREATION
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/windows/selector.c PRE-CREATION
Diff: https://reviews.apache.org/r/18893/diff/
Testing
-------
rhel 6, Win7, all 64 bit for now
Thanks,
Cliff Jansen
Re: Review Request 18893: Extending PROTON-525 to Windows platforms
(selectors)
Posted by Rafael Schloming <rh...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18893/#review36663
-----------------------------------------------------------
Ship it!
Ship It!
- Rafael Schloming
On March 10, 2014, 4:04 p.m., Cliff Jansen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18893/
> -----------------------------------------------------------
>
> (Updated March 10, 2014, 4:04 p.m.)
>
>
> Review request for qpid and Rafael Schloming.
>
>
> Bugs: PROTON-529
> https://issues.apache.org/jira/browse/PROTON-529
>
>
> Repository: qpid
>
>
> Description
> -------
>
> This patch is a minimal port of the existing Windows code base to incorporate the refactored IO and selector mechanism of PROTON-525. It does not incorporate any changes to how the sockets and "pipes" operate but some thought was given to accommodating completion ports right after 0.7.
>
> Although there was a clear attempt in PROTON-525 to provide rich abstractions that would allow Posix and Windows implementations to optimize their low level IO for each OS, awkwardness remains.
>
> Windows sockets are outside the regular IO system, for a long time a bolt on third party implementation. For Posix, they are an extension of regular file descriptors and things like close(socket_fd) makes sense.
>
> This short term Windows fix hides the above. Because the "sockets only" select() call in the Windows api was the easiest to work with initially, it was necessary to fake a socket-based pipe for the control port. Since everything is sockets anyway, we can mix the control fd with other fds. If we used Windows pipes, we couldn't. But we definitely don't want to use select() long term and we may not want to use fake socket pipes, or even any pipes at all.
>
> Note that the pipe control mechanism is not needed on Windows if you use completion ports and is not used in the Qpid C++ Windows implementation.
>
> Even though it is probable that the existing new api could be made to work without penalizing Windows significantly, I would prefer to stop mingling sockets with non socket io objects in proton.
>
> Perhaps some interrupt interface could be added to the selector, implemented with pipes on (some) Posix systems and optimized in other ways on other platforms.
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/CMakeLists.txt 1575326
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/include/proton/cproton.i 1575326
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/include/proton/io.h 1575326
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/include/proton/selectable.h 1575326
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/messenger/messenger.c 1575326
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/posix/driver.c 1575326
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/posix/io.c 1575326
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/selectable.h 1575326
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/selectable.c 1575326
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/ssl/ssl-internal.h 1575326
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/ssl/ssl_stub.c 1575326
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/windows/driver.c 1575326
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/windows/io.c PRE-CREATION
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/windows/selector.c PRE-CREATION
>
> Diff: https://reviews.apache.org/r/18893/diff/
>
>
> Testing
> -------
>
> rhel 6, Win7, all 64 bit for now
>
>
> Thanks,
>
> Cliff Jansen
>
>
Re: Review Request 18893: Extending PROTON-525 to Windows platforms
(selectors)
Posted by Cliff Jansen <cl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18893/
-----------------------------------------------------------
(Updated March 10, 2014, 4:04 p.m.)
Review request for qpid and Rafael Schloming.
Changes
-------
fixes 1,2, and 4. driver.c does not contain any use of selectors. So I will have to rework it to do that before it can be OS agnostic. I'm not sure that will be ready later today for the first RC. I'm also unsure how to test the change now that messenger doesn't use it.
Bugs: PROTON-529
https://issues.apache.org/jira/browse/PROTON-529
Repository: qpid
Description
-------
This patch is a minimal port of the existing Windows code base to incorporate the refactored IO and selector mechanism of PROTON-525. It does not incorporate any changes to how the sockets and "pipes" operate but some thought was given to accommodating completion ports right after 0.7.
Although there was a clear attempt in PROTON-525 to provide rich abstractions that would allow Posix and Windows implementations to optimize their low level IO for each OS, awkwardness remains.
Windows sockets are outside the regular IO system, for a long time a bolt on third party implementation. For Posix, they are an extension of regular file descriptors and things like close(socket_fd) makes sense.
This short term Windows fix hides the above. Because the "sockets only" select() call in the Windows api was the easiest to work with initially, it was necessary to fake a socket-based pipe for the control port. Since everything is sockets anyway, we can mix the control fd with other fds. If we used Windows pipes, we couldn't. But we definitely don't want to use select() long term and we may not want to use fake socket pipes, or even any pipes at all.
Note that the pipe control mechanism is not needed on Windows if you use completion ports and is not used in the Qpid C++ Windows implementation.
Even though it is probable that the existing new api could be made to work without penalizing Windows significantly, I would prefer to stop mingling sockets with non socket io objects in proton.
Perhaps some interrupt interface could be added to the selector, implemented with pipes on (some) Posix systems and optimized in other ways on other platforms.
Diffs (updated)
-----
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/CMakeLists.txt 1575326
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/include/proton/cproton.i 1575326
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/include/proton/io.h 1575326
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/include/proton/selectable.h 1575326
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/messenger/messenger.c 1575326
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/posix/driver.c 1575326
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/posix/io.c 1575326
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/selectable.h 1575326
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/selectable.c 1575326
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/ssl/ssl-internal.h 1575326
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/ssl/ssl_stub.c 1575326
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/windows/driver.c 1575326
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/windows/io.c PRE-CREATION
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/windows/selector.c PRE-CREATION
Diff: https://reviews.apache.org/r/18893/diff/
Testing
-------
rhel 6, Win7, all 64 bit for now
Thanks,
Cliff Jansen
Re: Review Request 18893: Extending PROTON-525 to Windows platforms
(selectors)
Posted by Rafael Schloming <rh...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18893/#review36509
-----------------------------------------------------------
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/messenger/messenger.c
<https://reviews.apache.org/r/18893/#comment67501>
My thinking with the pn_io_t was to use it to avoid implicit use of thread locals. To do this I would think we'd want pn_io_wouldblock() to take pn_io_t as an argument.
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/posix/io.c
<https://reviews.apache.org/r/18893/#comment67502>
See comment above about taking a pn_io_t * as an argument to this.
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/windows/driver.c
<https://reviews.apache.org/r/18893/#comment67503>
Do we need two different driver.c files now? Shouldn't the same one work now for both windows and posix?
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/windows/io.c
<https://reviews.apache.org/r/18893/#comment67504>
I've been trying to standardize on "pni_" for an internal prefix. We do still use "pn_i_", but I believe it is in the minority now.
- Rafael Schloming
On March 7, 2014, 5:48 a.m., Cliff Jansen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18893/
> -----------------------------------------------------------
>
> (Updated March 7, 2014, 5:48 a.m.)
>
>
> Review request for qpid and Rafael Schloming.
>
>
> Bugs: PROTON-529
> https://issues.apache.org/jira/browse/PROTON-529
>
>
> Repository: qpid
>
>
> Description
> -------
>
> This patch is a minimal port of the existing Windows code base to incorporate the refactored IO and selector mechanism of PROTON-525. It does not incorporate any changes to how the sockets and "pipes" operate but some thought was given to accommodating completion ports right after 0.7.
>
> Although there was a clear attempt in PROTON-525 to provide rich abstractions that would allow Posix and Windows implementations to optimize their low level IO for each OS, awkwardness remains.
>
> Windows sockets are outside the regular IO system, for a long time a bolt on third party implementation. For Posix, they are an extension of regular file descriptors and things like close(socket_fd) makes sense.
>
> This short term Windows fix hides the above. Because the "sockets only" select() call in the Windows api was the easiest to work with initially, it was necessary to fake a socket-based pipe for the control port. Since everything is sockets anyway, we can mix the control fd with other fds. If we used Windows pipes, we couldn't. But we definitely don't want to use select() long term and we may not want to use fake socket pipes, or even any pipes at all.
>
> Note that the pipe control mechanism is not needed on Windows if you use completion ports and is not used in the Qpid C++ Windows implementation.
>
> Even though it is probable that the existing new api could be made to work without penalizing Windows significantly, I would prefer to stop mingling sockets with non socket io objects in proton.
>
> Perhaps some interrupt interface could be added to the selector, implemented with pipes on (some) Posix systems and optimized in other ways on other platforms.
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/CMakeLists.txt 1573021
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/include/proton/cproton.i 1573021
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/include/proton/io.h 1573021
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/include/proton/selectable.h 1573021
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/messenger/messenger.c 1573021
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/posix/driver.c 1573021
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/posix/io.c 1573021
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/selectable.h 1573021
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/selectable.c 1573021
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/ssl/ssl-internal.h 1573021
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/ssl/ssl_stub.c 1573021
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/windows/driver.c 1573021
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/windows/io.c PRE-CREATION
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/windows/selector.c PRE-CREATION
>
> Diff: https://reviews.apache.org/r/18893/diff/
>
>
> Testing
> -------
>
> rhel 6, Win7, all 64 bit for now
>
>
> Thanks,
>
> Cliff Jansen
>
>