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

Better to use an NSThread than a global dispatch queue #15

Open
snej opened this issue Apr 7, 2015 · 13 comments
Open

Better to use an NSThread than a global dispatch queue #15

snej opened this issue Apr 7, 2015 · 13 comments

Comments

@snej
Copy link

snej commented Apr 7, 2015

JetFire runs its background stream handler by dispatching a block to a system concurrent dispatch queue, starting a runloop, and running it indefinitely until the socket closes. This works, but I believe it's bad form for a couple of reasons:

  1. The system dispatch queues are supposed to be used for short tasks and shouldn't be blocked for large amounts of time.
  2. You're creating a runloop on a thread you don't own. And the runloop remains in existence (but idle) after you're done with it, since the thread will be reused by other tasks afterwards.

I think it would be safer/cleaner to simply start an NSThread instead. It's a simple change — just call +[NSThread detachNewThreadSelector:...] instead of dispatch_async.

Even better is the approach used by PocketSocket: share one background thread/runloop to receive NSStream events for all sockets. (But it only makes a difference if a lot of sockets will be open at once.)

[Note: I haven't used JetFire yet; I'm still kicking the tires of various Cocoa WebSocket libraries.]

@adamkaplan
Copy link
Contributor

I wandered over here to make a similar comment. Totally agree that spinning a global runloop is bad for business. I was going to suggest an owned dispatch queue, but NSThread is actually a better fit because you don't have to worry about eating into GCDs thread pool.

@acmacalister
Copy link
Owner

sorry for the delay. I agree an NSThread would be a better call. I remember reading that dispatch queues where supposed to be used for definite timed tasks as well, but I can't seem to find those docs anywhere. if you happen to have a link to those, I would love to add it. I will try and get that change updated today.

@adamkaplan
Copy link
Contributor

@acmacalister It's not really about the lifetime of the task and I doubt you'll find it written as such. The underlying concern is that GCD is a magical box that executes tasks on a pool of threads. It's assumed that all tasks will be finite in duration (otherwise why use a task queue?). The underlying thread that you execute on with GCD is fairly arbitrary.

Even if you create a custom "queue", you're still eating GCDs thread pool. I want to suggest checking out dispatch_io, which is an I/O focused subset of GCD that might serve this project better.

Using your own NSThread will guarantee that you operate safely, but on the downside you're dedicating a real, scheduled POSIX real thread to each WebSocket – which isn't ideal in a world where GCD is available.

I have some ideas and intend on fixing some of these issues

@snej
Copy link
Author

snej commented Apr 10, 2015

on the downside you're dedicating a real, scheduled POSIX real thread to each WebSocket

As I mentioned above, PocketSocket uses a shared singleton thread to provide a runloop to schedule all of the streams on. That seems the best approach if one's using NSStream/CFStream.

I want to suggest checking out dispatch_io, which is an I/O focused subset of GCD that might serve this project better.

It's very attractive as a low-level interface that fits in very well with dispatch queues; the downside is that you lose out on some of the stuff that CFStream gives you for free, like SSL support. (You can support SSL by calling SecureTransport directly, as GCDAsyncSocket does, but it looks fairly complex.)

@acmacalister
Copy link
Owner

@adamkaplan Thanks for the writeup. I understand the difference between GCD and how posix threads work. I get that GCD is a thread pool built off libdispatch 😄 . I seem to remember many years ago reading docs that had recommendations on when to use GCD over more traditional model, like NSThread. That could just be my poor recollection though.

I will look into dispatch_io, but the way I understand it, we are still having to use a thread out of GCD's thread pool regardless, which we don't actually have control over, which brings up the point of safety that @snej mentioned above.

I agree the thread per WebSocket is not ideal either and a singleton setup in a reactor pattern style would be a nice compromise. Overall I appreciate the feedback and is a big reason I open sourced the code in the first place. Let me know what you guys thinks.

@snej
Copy link
Author

snej commented Apr 10, 2015

No, dispatch_io doesn't dedicate a thread. It's an event-based API that's part of GCD. It lets you schedule a block to run on your dispatch queue when a stream receives data or has space available for writes.

@adamkaplan
Copy link
Contributor

@snej What about something like...

CFDataRef writeSocketData = CFWriteStreamCopyProperty(stream, kCFStreamPropertySocketNativeHandle);

CFSocketNativeHandle writeHandle;
CFDataGetBytes(writeSocketData, CFRangeMake(0, sizeof(CFSocketNativeHandle)), (UInt8*)&writeHandle);

dispatch_io_t writeIo = dispatch_io_create(DISPATCH_IO_STREAM, (int)writeHandle, (__bridge dispatch_queue_t)clientCallBackInfo, ^(int error) {
  //
});

And something similar for the readHandle?

I hacked a prototype together. It does return something, but I haven't fully proved it yet.

@snej
Copy link
Author

snej commented Apr 10, 2015

I'm pretty certain that by doing that you're bypassing/breaking SSL. Normally if you enable SSL on a CFStream, when you call CFWrite it calls SecureTransport which encrypts the data and then writes that to the socket.

@adamkaplan
Copy link
Contributor

Makes sense

@daltoniam
Copy link
Collaborator

I did #16 implementing this with a shared singleton thread. Let me know what you all thing.

@snej
Copy link
Author

snej commented Apr 13, 2015

Marc Krochmal on the macnetworkprog mailing list just clued me in to the functions CFReadStreamSetDispatchQueue and CFWriteStreamSetDispatchQueue, which do exactly what they say. With these you don't need to do anything funky with threads or runloops — your stream callbacks / delegate methods will get called on whatever queue you want. Brilliant.

@adamkaplan
Copy link
Contributor

😲 I noticed those properties and but nothing clicked in my head, nice find!

@acmacalister
Copy link
Owner

@snej, awesome! I will give those a try.

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

No branches or pull requests

4 participants