acmacalister / jetfire

WebSocket (RFC 6455) client library for iOS & OS X
Apache License 2.0
518 stars 83 forks source link

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

Open snej opened 9 years ago

snej commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

@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 commented 9 years ago

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 commented 9 years ago

@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 :smile: . 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 commented 9 years ago

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 commented 9 years ago

@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 commented 9 years ago

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 commented 9 years ago

Makes sense

daltoniam commented 9 years ago

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

snej commented 9 years ago

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 commented 9 years ago

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

acmacalister commented 9 years ago

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