[BUGFIX] Avoid common output stream setup errors, including runloop scheduling #507

Closed
wants to merge 1 commit into
from

Projects

None yet

3 participants

@sptramer

This commit fixes two bugs with setting the output stream on an URLConnectionOperation, one common and one not:

  • The less common bug is setting outputStream to the same value twice. If this is done then the value of outputStream should not be triggered as updated, and there's also a very good chance that the stream should not be closed, either. If this fix isn't appropriate I can back it out.
  • The more common (and dangerous) bug is runloop scheduling. Output streams may be scheduled in multiple runloops according to the documentation. This can create contention between active runloops for performing operations with the stream. Even worse, if the operation is created on (and the outputStream is set on) the main thread, this may block main thread activity ESPECIALLY with networks that have high latency or dropped packets (such as Edge or 3G) resulting in a SIGKILL from the OS. Could also cause (possibly severe and hard to diagnose) crashes if an operation were created and scheduled in, say, a callback of another operation.

This fix should resolve some of the "3G crash" issues people seem to experience.

Stephen Tramer [BUGFIX] Avoid common output stream setup errors, including immediate…
…ly scheduling the stream on a runloop.
9ffd222
@mattt
mattt commented Sep 14, 2012

Your first point is absolutely correct, but removing the runloop scheduling in the way that you are there will cause other problems, as outputStream is used internally in several places, and does need to be scheduled. I can refactor that to make it play better with user-defined output streams.

To your point about this fixing the 3G crash, I strongly doubt that this is related. By all accounts, the crash there is related to the amount of data being streamed on upload, which is apparently fixed by throttling the NSInputStream.

a4508ac pulls in the changes suggested in this pull request in a way that maintains existing functionality.

@mattt mattt closed this Sep 14, 2012
@sptramer

It's actually a problem with response delay times - it just so happens that the most common delay with 3G is the upload rather than download stream. Competing runloop scheduling was apparently causing some problems there when I tested under the network link conditioner.

@egrim egrim pushed a commit to egrim/AFNetworking that referenced this pull request Sep 18, 2012
@mattt mattt [Issue #507] Fixing potential issues with output stream runloop sched…
…uling
4a98dd0
@couchdeveloper

I too doubt that 3G issues stem from the fact that the output stream in AF is scheduled in a problematic way. In fact, in case of downloading data there aren't delegate methods which will be scheduled at all:

When looking into method -connection:didReceiveData: scheduling the output stream on any run loop has no effect - since there is also no stream delegate whose methods could be executed on the run loop. Instead, within the method, the stream receives its data directly through method -write:maxLength: -- which is executed on the private shared thread.

IMO, the code which "attaches" the output stream to a run loop can be omitted.

If however, the output stream becomes publicly accessible - say the user of the AF library can set the output stream, the code in method connection:didReceiveData: must be revised since it currently does only work for specialized streams, namely memory streams and file streams, which always do successfully write as many bytes have been requested. This is not true for custom and other streams, though.

Furthermore, the attempt to schedule ouput-stream delegate methods on a certain run loop in conjunction with a NSURLConnection makes no sense at all, even if this is a custom stream. In that case, the connection would be the output-stream's delegate. But due to the imperative, say pro-active, data producer (which is the connection and which is the stream's delegate) there is no way to interface an output stream with the connection via stream events and delegates. It would require a different paradigm. In other words, the connection says "Here is one chunk of data, process it immediately!", and it is not that the connection receives "Hi, output-stream here. Please give me more data!". ;)

In case of an upload, the NSURLRequest's body property will be assigned an input-stream.
(side note: in case the request body is an input-stream, NSURLConnection will use "Transfer-Encoding: Chunked").
The NSURLRequest object will set itself as the delegate for the input-stream. The input stream should also not be opened, prior to assigning it to the request body. I don't know the effect of scheduling an input stream on any run loop prior to setting it as the body stream of a NSURLRequest. I suspect, the NSURLRequest will - if at all - schedule the input stream on the "connection" thread. However, if I were the implementor, I would schedule it on a separate and private thread in order to avoid possible dead locks.

Edit: it seems, the request body input stream delegates will be scheduled on the thread "com.apple.NSURLConnectionLoader".

@greghe greghe pushed a commit to skillz/AFNetworking that referenced this pull request Sep 3, 2015
@mattt mattt [Issue #507] Fixing potential issues with output stream runloop sched…
…uling
7bde506
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment