Bonjour support #24

Merged
merged 13 commits into from Jan 12, 2013

Projects

None yet

3 participants

@jeanregisser
Contributor

This adds support for auto discovery of the ponyd server via Bonjour.

So instead of specifying a host to connect to in the client code, you can just do:

[debugger autoConnect];

Let me know what you think.

Note: I've opened this new pull request because I had inadvertently specified a sha1 in #19 instead of a branch.

@mikelikespie mikelikespie and 1 other commented on an outdated diff Oct 26, 2012
ponyd/gateway.py
@@ -233,6 +240,8 @@ def __call__(self):
print "PonyGateway starting. Listening on %s:%s" % (self.listen_interface, self.listen_port)
+ thread.start_new_thread(bonjour.register_service, (self.bonjour_name, "_ponyd._tcp", self.listen_port))
@mikelikespie
mikelikespie Oct 26, 2012 Collaborator

Is there a way we can do this without using threads? If not, that's OK, but, the server uses Tornado which is event driven. I just checked out pybonjour and it looks like it can work with select (which is how the ioloop in tornado works internally)

Please check out http://www.tornadoweb.org/documentation/ioloop.html

Here's what I think you'll need to do

io_loop.add_handler(service,  some_callback_to_dnsserviceprocessresult, io_loop.READ)

This means you won't have to run a select loop yourself.

@jeanregisser
jeanregisser Oct 28, 2012 Contributor

Ok I'll see what I can do with the ioloop. I just went with the simplest way to add this feature.

@mikelikespie mikelikespie and 1 other commented on an outdated diff Oct 26, 2012
ObjC/PonyDebugger/PDDebugger.m
@@ -177,6 +182,71 @@ - (void)webSocket:(SRWebSocket *)webSocket didFailWithError:(NSError *)error;
_socket = nil;
}
+#pragma mark - NSNetServiceBrowserDelegate
+
+- (void)netServiceBrowser:(NSNetServiceBrowser*)netServiceBrowser didFindService:(NSNetService*)service moreComing:(BOOL)moreComing;
+{
+ if (_bonjourServiceName
+ && NSOrderedSame != [_bonjourServiceName compare:service.name
+ options:NSCaseInsensitiveSearch | NSNumericSearch | NSDiacriticInsensitiveSearch]) {
+ return;
+ }
@mikelikespie
mikelikespie Oct 26, 2012 Collaborator

Please format to something like

const NSStringCompareOptions compareOptions = NSCaseInsensitiveSearch | NSNumericSearch | NSDiacriticInsensitiveSearch;
if (_bonjourServiceName  != nil  && [_bonjourServiceName compare:service.name options:compareOptions] != NSOrderedSame) {

Apologies for not having a style guide.

But also, why NSNumericSearch?

@jeanregisser
jeanregisser Oct 28, 2012 Contributor

You're right NSNumericSearch is not needed, I don't remember why I put it ;)

@mikelikespie mikelikespie commented on an outdated diff Oct 26, 2012
Examples/PDTwitterTest/PDTwitterTest/PDAppDelegate.m
@@ -46,8 +46,12 @@ - (BOOL)application:(UIApplication *)application didFinishLaunchingWithOptions:(
[debugger enableCoreDataDebugging];
[debugger addManagedObjectContext:self.managedObjectContext withName:@"Twitter Test MOC"];
- // Connect on launch.
- [debugger connectToURL:[NSURL URLWithString:@"ws://localhost:9000/device"]];
+ // Auto connect via bonjour discovery
+ [debugger autoConnect];
+ // Or to a specific ponyd bonjour service
+ //[debugger autoConnectToBonjourServiceNamed:@"MY PONY"];
@mikelikespie
mikelikespie Oct 26, 2012 Collaborator

Would prefer to leave the the default to the direct hostname for now. Would like to think more about security implications, etc, and having it a bit battle tested before changing the defaults.

@mikelikespie mikelikespie commented on the diff Oct 26, 2012
ObjC/PonyDebugger/PDDebugger.m
+ NSUInteger serviceIndex = [_bonjourServices indexOfObject:service];
+ if (NSNotFound != serviceIndex) {
+ [_bonjourServices removeObjectAtIndex:serviceIndex];
+ NSLog(@"Removed ponyd bonjour service: %@", service);
+
+ // Try next one
+ if (!_currentService && _bonjourServices.count){
+ NSNetService* nextService = [_bonjourServices objectAtIndex:(serviceIndex % _bonjourServices.count)];
+ [self _resolveService:nextService];
+ }
+ }
+}
+
+#pragma mark - NSNetServiceDelegate
+
+- (void)netService:(NSNetService *)service didNotResolve:(NSDictionary *)errorDict;
@mikelikespie
mikelikespie Oct 26, 2012 Collaborator

Will this keep stop after exhaustively trying all the services, or keep looping? (just need clarification, no fixes)

@jeanregisser
jeanregisser Oct 28, 2012 Contributor

It will loop on all services until one of them can be resolved.

@mikelikespie mikelikespie commented on the diff Oct 26, 2012
ponyd/bonjour.py
@@ -0,0 +1,27 @@
+import select
+import pybonjour
+import logging
+
+logger = logging.getLogger('bonjour')
+
+def register_service(name, regtype, port):
@mikelikespie
mikelikespie Oct 26, 2012 Collaborator

See comment below re: threads

@mikelikespie mikelikespie commented on an outdated diff Oct 26, 2012
ponyd/bonjour.py
@@ -0,0 +1,27 @@
+import select
+import pybonjour
+import logging
+
+logger = logging.getLogger('bonjour')
+
+def register_service(name, regtype, port):
+ def register_callback(sdRef, flags, errorCode, name, regtype, domain):
+ if errorCode == pybonjour.kDNSServiceErr_NoError:
+ logger.debug('Registered bonjour service %s.%s', name, regtype)
+
+ service = pybonjour.DNSServiceRegister(name = name,
@mikelikespie
mikelikespie Oct 26, 2012 Collaborator

For named arguments, I prefer foo=bar. This is consistent with PEP8 (even though not really following it for everything)

@mikelikespie mikelikespie commented on an outdated diff Oct 26, 2012
ponyd/bonjour.py
+import pybonjour
+import logging
+
+logger = logging.getLogger('bonjour')
+
+def register_service(name, regtype, port):
+ def register_callback(sdRef, flags, errorCode, name, regtype, domain):
+ if errorCode == pybonjour.kDNSServiceErr_NoError:
+ logger.debug('Registered bonjour service %s.%s', name, regtype)
+
+ service = pybonjour.DNSServiceRegister(name = name,
+ regtype = regtype,
+ port = port,
+ callBack = register_callback)
+
+ try:
@mikelikespie
mikelikespie Oct 26, 2012 Collaborator

Even though I think this will be going away to use the ioloop, you could combine the two tries without restructuring much.

@mikelikespie
Collaborator

@jeanregisser Thanks for the pull request! It mostly looks good aside from a few comments. Once they're resolved, I will get it merged in.

As an aside, I think a nice addition would be a delegate that you could hook up to the resolver that will allow one to easily hook it up to a UITableView for an endpoint selection menu. It will be hard to make it secure without a UI to validate who you're connecting to. It may not be a concern now, but I have plans on adding remote execution via JSCocoa eventually.

@jeanregisser
Contributor

Thanks for the feedback!
I'll make the necessary changes in the coming days.

@jeanregisser
Contributor

I've finished the necessary changes.
Any chance to merge this soon?

@mathieuravaux

+1 this is really convenient

@mikelikespie
Collaborator

Cool! This looks much better (python runloop stuff in particular). Sorry for the delay getting it merged.

@mikelikespie mikelikespie merged commit 137cdf4 into square:master Jan 12, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment