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 CFStream support #245

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

Add CFStream support #245

wants to merge 1 commit into from

Conversation

mackross
Copy link

I've created a concrete "socket" class that uses CFStream for iOS (& perhaps OS X? will need to check).

On iOS we get a few extras such as: no polling (streams are attached to a runloop), radio gear will start up if required, option to send data via wifi-only, and native SSL.

Seems to be working without any show stopping bugs at the moment, but I'll clean up a few things and expose a few more features as I continue to integrate it into the librabbitmq-objc wrapper I'm overhauling (renamed to AMQPKit) http://github.com/mackross/AMQPKit.

Lastly, I'm not at all familiar with cmake & iOS I was hoping someone else could give me some advice as I'm pretty sure I have broken the normal build process (I'm using the Xcode project in AMQPKit).

Review on Reviewable

@@ -737,7 +737,7 @@ typedef enum {

AMQP_END_DECLS

#include <amqp_framing.h>
#include "amqp_framing.h"
Copy link
Owner

Choose a reason for hiding this comment

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

Leave this as angle brackets please.

Copy link
Author

Choose a reason for hiding this comment

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

Had to change this as it won't compile in obj-c without "".

Copy link
Author

Choose a reason for hiding this comment

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

actually, with some xcode hackery was able to fix.

@alanxz
Copy link
Owner

alanxz commented Feb 24, 2015

Interesting addition. I'll need to do a more thorough review at some point in the near future (hopefully sometime this week). I'd recommend doing a rebase against master - which will help you get rid of that merge commit in the PR.

I personally haven't tried to build rabbitmq-c for iOS (though I know others have). I'll have to do a bit of investigation as to how this is done.

Additionally, there's a (unpublished) roadmap to provide a better abstraction for the transport layer (right now the socket-timeout stuff is somewhat intertwined with the rest of the library). The eventual goal is to allow developers to bring their own event-loop/transport backend (like libuv, CFstream, ASIO, etc), and do a better job providing an API to use AMQP.

@mackross
Copy link
Author

Thanks for the feedback, and there is no hurry on the review. I'm tied up with a few other things for a while (using the client 👍), but I'll keep an eye on your comments and push over things as they get cleaned up.

On the abstraction for the transport layer. Sounds like a good idea. Happy to give feedback if you want.

@mackross mackross force-pushed the master branch 2 times, most recently from bca11ea to fd3cff4 Compare February 24, 2015 08:54
@ghost
Copy link

ghost commented Feb 24, 2015

This is something a LOT of people want and need. RabbitMQ on iOS is a hack job with 2 year old wrappers, etc. It would be incredible to have true iOS support.

@ghost
Copy link

ghost commented Feb 24, 2015

To compile it for iOS, make sure you comment out the following sections on amp_private.h otherwise it throws errors in Xcode(don't know about command line builds):

Determine byte order around line 254
Down to around line 332(lines above it are the ifndef HAVE_HTONLL which should be commented out)

The DECLARE_CODEC_BASE_TYPE lines need to still be there. I also make adjustments to take out ampq_abort() from AMQP_NORETURN and I also remove the exit() call of that function because it'll take down your whole iOS app if you let it call exit() when a crash happens. I use the wrapper to catch any errors and automatically recover by restarting the entire rabbitMQ subsystem.

I do NOT statically pre-compile the library and put it into the project. It is compiled along with the entire project on build so I actually can see debug information inside the rabbitMQ-c client itself when issues happen instead of it pointing to the library and shrugging at me..

@alanxz
Copy link
Owner

alanxz commented Feb 25, 2015

@HT-Stephen as described previously to you in #206: it is not necessary to comment the htonll stuff out. Having HAVE_HTONLL defined in the preprocessor will make rabbitmq-c compile for iOS without modifying the source code.

@ghost
Copy link

ghost commented Feb 25, 2015

Yea you are absolutely right about that. I totally forgot!! I'm going to make that change now to my own code so I no longer have to do that. I would prefer to pull the rabbitMQ-c source cleanly without having to do any custom modifications just for my usage scenario. Thank you for the reminder!! :)

@alanxz
Copy link
Owner

alanxz commented Mar 6, 2015

  • Would you mind breaking up the one commit into some smaller commits? It'll make this PR a little easier to review for me (600 lines its hard to digest). You can leave it all in 1 PR.
  • Have you considered using CFHost to do the name-resolution instead of getaddrinfo() (I do realize this ups the complexity here a bit). I've read somewhere that you need to use CFHost in order to correctly wake up the radio when its asleep to do name resolution (which getaddrinfo() does not do).

Other things that you don't necessarily need to do in this PR, but I need to consider at some point in the near future if I don't want this code to quickly bit-rot:

  • Need to figure out how to add iOS to the continuous integration (travis-ci, something else?).
  • Need to make it so CMake can be used to generate a build-system that works.

@mackross
Copy link
Author

mackross commented Mar 6, 2015

  • I'm be happy to break it up, but I think commenting it will be of better use. There isn't really logical places to break up the commits as it's all needed for functionality (and only 1 modified file and its only a few lines). It might be a week or so before I get a chance to do this though.

  • This concrete socket implementation never actually calls getaddrinfo() as amqp_open_sock_noblock is not called from within the socket open function like the other implementations -- Instead CFStreamCreatePairWithSocketToHost (line 234) is used which internally uses CFHost to do name-resolution when either the read or write streams are opened (line 268). If the CFHost name-resolution fails, an error is reported on a future runloop iteration through amqp_cfstream_socket_write_stream_event_handler or amqp_cfstream_socket_read_stream_event_handler and can be copied from the streams as an NSError.

    I had planned to add a method to the socket for getting NSErrors that occur on the streams (including name-resolution failure). At the moment any stream error it is just reported as an amqp socket error and the NSError is logged.

    Regarding using CFHost directly, I believe that we could split the name-resolution from opening the stream up by using CFHost directly and then using CFStreamCreatePairWithSocketToCFHost instead of CFStreamCreatePairWithSocketToHost. This would allow us to block the socket open function until name-resolution completes or errors. I'm not sure this is the desired behaviour though.

  • Both good ideas. I was unable to find a CMake toolchain that worked correctly for iOS, so I just created an Xcode project from scratch and manually imported the files and set the build flags. I'm afraid I won't be much help with CMake, but I might be able to help with travis if we're able to either create a building Xcode project or add one.

@alanxz
Copy link
Owner

alanxz commented May 4, 2015

Quick update: I have been able to make rabbitmq-c build unmodified for iOS with project files generated from CMake. The missing pieces were: a fix that was introduced in CMake v3.2.1, and adding a couple lines to the ios-cmake Toolchain file (See alanxz/ios-cmake, specifically alanxz/ios-cmake@4c312bd).

The next step is finding a place to host a CI build of rabbitmq-c targeting iOS. I missed the boat on signups for travis-ci allowing more than one OS, same deal for CircleCI, so I'm still looking for a good way to do this. If we can get this issue rectified, I think that this can be shaped up and put into the v0.7.0 release.

@mackross
Copy link
Author

@alanxz
Copy link
Owner

alanxz commented May 19, 2015

Both ship.io and greenhouseci.com seem to require that xcode project files be checked in (there's no way to invoke CMake to generate these).

@alanxz alanxz closed this Oct 14, 2015
@alanxz alanxz reopened this Oct 14, 2015
@carlhoerberg
Copy link

If there's a fee for TravisCI/CircleCI or so that you want to use I'm happy to pay for it

@alanxz
Copy link
Owner

alanxz commented Mar 16, 2017

Since this PR was opened, travis now allows me to build on osx, which includes the ability to do ios builds, no need to for circle-ci.

This PR is now sufficiently old that it likely needs some refactoring to get back to a reviewable state. If someone is willing to bring this back to a state where it can be reviewed, I'm willing to add this to rabbitmq-c.

@auvipy
Copy link

auvipy commented Jan 11, 2023

if this do not have any taker let me now

@mackross
Copy link
Author

All yours

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.

4 participants