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

[READY] hs.socket #791

Merged
merged 44 commits into from
Mar 3, 2016
Merged

[READY] hs.socket #791

merged 44 commits into from
Mar 3, 2016

Conversation

heptal
Copy link
Contributor

@heptal heptal commented Feb 12, 2016

Adds hs.socket for communicating with arbitrary protocols using CocoaAsyncSocket.

TCP sockets are available under hs.socket
UDP sockets are available under hs.socket.udp


static const char *USERDATA_TAG = "hs.socket";

int refTable;
Copy link
Member

Choose a reason for hiding this comment

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

(This isn't documented anywhere yet, or even widespread convention, but) this should be static too

return [super initWithDelegate:self delegateQueue:dispatch_get_main_queue()];
}

- (void)socket:(HSAsyncSocket *)sock didConnectToHost:(NSString *)host port:(UInt16)port {
Copy link
Member

Choose a reason for hiding this comment

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

Is this guaranteed to be called on the main thread by GCDASyncSocket? I am guessing it must be, otherwise you would have had failures from LuaSkin when it noticed it was being accessed from a non-main thread, but it seems like everything else here is expecting to be happening on random threads.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. You can specify your own background serial queue like:

- (id)init {
    dispatch_queue_t socketQueue = dispatch_queue_create("socketQueue", NULL);
...
    return [super initWithDelegate:self delegateQueue:socketQueue];
}

Then, in each of the delegate methods that operate on the UI/Lua in any way, you must wrap those operations like so:

    dispatch_async(dispatch_get_main_queue(), ^{ 
        @autoreleasepool {
            LuaSkin *skin = [LuaSkin shared];
            [skin logInfo:@"Data written to socket"];
            // do some lua callback or something
        }
    });

Failing to dispatch_async to the main thread will indeed crash as you say. I initially chose dispatch_get_main_queue as the delegateQueue because it 'just worked', but it does seem a bit more performant if I use background queues and call dispatch_async to the main thread as shown above, presumably because GCDAsyncSocket avoids doing all of its internal operations on the main thread, waiting until the results are in. I have an uncommitted implementation of this and wanted your opinion before really putting it in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How feasible/difficult would it be to have LuaSkin's log* methods automatically call dispatch_async to the main thread for log operations performed on background threads, with everything else being subjected to the if (![NSThread isMainThread]) check performed in + (id)shared? Seems to me like that wouldn't be terribly unsafe, but might be too much work for not a lot of benefit.

In any case I've converted it to use the background queue with dispatch_async to the main thread where needed and it passes all tests.

Copy link
Member

Choose a reason for hiding this comment

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

The most reliable way to do this is to add class methods for the logX: methods... anything that relies on [LuaSkin shared] will be rejected before the method is ever entered. (e.g. [LuaSkin logWarn:...] rather than/in addition to [[LuaSkin shared] logWarn:...]

It's not a bad idea, maybe even include a prefix indicating when it's from another thread; @cmsj objections to my adding this? I'd also like to see hs.logger used in the Lua side of the output so verbose and debug can be turned on and off as desired and can add that at the same time.

@cmsj
Copy link
Member

cmsj commented Feb 23, 2016

@heptal this is looking really good. I commented with a few little bits and bobs, but on the whole I am super pleased with this PR! Thanks very much :)

@heptal
Copy link
Contributor Author

heptal commented Mar 3, 2016

I think this is ready. It exposes pretty much everything in CocoaAsyncSocket.

hs.socket at 99% coverage
hs.socket.udp at 96% coverage
Uncovered bits are errors that I don't know how to induce (and presumably are quite rare).

@heptal heptal changed the title [WIP] hs.socket [READY] hs.socket Mar 3, 2016
heptal added 26 commits March 3, 2016 07:44
…serdata_gc, change registerLibraryWithObject to separate registerLibrary/registerObject
cmsj added a commit that referenced this pull request Mar 3, 2016
@cmsj cmsj merged commit d864cba into Hammerspoon:master Mar 3, 2016
@heptal heptal deleted the socket branch March 10, 2016 01:43
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.

3 participants