Completion block fix - #49
Conversation
| func bodyReceived(data: UnsafePointer<Int8>?, length: Int) -> Int32 { | ||
| processCurrentCallback(.bodyReceived) | ||
| guard let data = data else { return 0 } | ||
| if shouldStopProcessingBody { |
There was a problem hiding this comment.
Optimally, this should be before the assignment.
There was a problem hiding this comment.
Locking the semaphore to check the Bool value should take precedence over the nil-check?
There was a problem hiding this comment.
Sorry, I'm getting ahead of myself. I'm not convinced we need any lock protection around a stop flag. We're dealing with two serial queues - one reader and one writer. The only logic that runs on the writer queue is the writeXXXX() calls, so if reads and updates to the stop flag are only by code executing on the reader queue then we don't need a lock.
If we don't need to lock it, it should optimally be before the nil check and assign (unless data = nil is a more common event?)
| // just passing in a pointer to the internal ivar. But that ivar can't be modified in | ||
| // more than one place, so we have to put a semaphore around it to prevent that. | ||
| _shouldStopProcessingBodyLock.wait() | ||
| handler(.chunk(data: chunk, finishedProcessing: {self._shouldStopProcessingBodyLock.signal()}), &_shouldStopProcessingBody) |
There was a problem hiding this comment.
&_shouldStopProcessingBody isn't protected by the lock....
There was a problem hiding this comment.
Do we actually need to wait for the async completion handler to run before we can set stop? Don't we want to set it immediately (sync) even if an async writeBody() is still going to happen?
There was a problem hiding this comment.
&_shouldStopProcessingBody isn't protected by the lock....
?
That's what line 326 does.
Do we actually need to wait for the async completion handler to run before we can set stop? Don't we want to set it immediately (sync) even if an async writeBody() is still going to happen
We can't set stop here - it's passed as in inout to the API, and it's set by the handler.
There was a problem hiding this comment.
The completion handler that releases the lock runs async (separate thread) - there's no guarantee that its going to call signal() after _shouldStopProcessingBody is modified, so its effectively unprotected.
I was wondering whether your intent was to block access to _shouldStopProcessingBody until after the completion handler has run.
|
Out of interest, is there any reason why stop would be a flag rather than an API call on the HTTPResponseWriter, i.e. alongside done() and abort()? |
It's not my API, I just implemented it. |
ianpartridge
left a comment
ianpartridge
left a comment
There was a problem hiding this comment.
Please break this down into one PR per problem, or a series of commits that build on each other and can be reviewed independently.
|
Removing in favor of #51 (and the several others that will be stacked on top of it) |
3 participants
Fix for completion block issue #36.
Also includes a fix for #48 that caused me to retract my last attempt (PR #47).
Also adds at least one test for #46 while I was at it (but more tests would still be useful there).