Cancel DispatchSource before closing socket (#4791)
#4859
Conversation
CURL documentation (https://curl.se/libcurl/c/CURLMOPT_TIMERFUNCTION.html) explicitly says that the timer should be one-time. We basically have to follow CURL requests for setting, resetting and disarming such timers. Current logic eventually leaves a 1ms repeating timer forever, because CURL assumes it fires once, and may not ask us to remove it explicitly. Also, being used as request timeout trigger, this timer also has no sense to be repeated.
lxbndr
changed the title
DispatchSource before closing socket (#4791)
lxbndr
force-pushed
the
readdle/urlsession-dispatchsource-close
branch
from
7e9e89d
to
bab881e
Compare
| self?.timeoutTimerFired() | ||
| } | ||
| timeoutSource = _TimeoutSource(queue: queue, milliseconds: milliseconds, handler: block) | ||
| //TODO: Could simply change the existing timer by using DispatchSourceTimer again. |
There was a problem hiding this comment.
Does it has to be done or one issue will be created ?
| //TODO: Could simply change the existing timer by using DispatchSourceTimer again. | |
| // TODO: Could simply change the existing timer by using DispatchSourceTimer again. |
There was a problem hiding this comment.
Can't really say on behalf on this comment author😞 This was here from the beginning. Guess it is better to leave as is to keep the diff less noisy.
There was a problem hiding this comment.
Yeah, let the Todo, I thought you've added it
| } | ||
| if action.needsWriteSource { | ||
| createWriteSource(socket: socket, queue: queue, handler: handler) | ||
| if (action.needsReadSource || action.needsWriteSource) { |
There was a problem hiding this comment.
Is () is really needed ?
| if (action.needsReadSource || action.needsWriteSource) { | |
| if action.needsReadSource || action.needsWriteSource { |
There was a problem hiding this comment.
Oh, how did it get here? Should've cleaned this. Thanks!
| @@ -99,7 +99,7 @@ class _TCPSocket: CustomStringConvertible { | |||
| listening = false | |||
| } | |||
|
|
|||
| init(port: UInt16?) throws { | |||
| init(port: UInt16?, backlog: Int32) throws { | |||
There was a problem hiding this comment.
I propagated backlog parameter from listen function signature, and it is Int32 there. I guess it is imported from C int, which is CInt in Swift, which, in turn, is the alias for Int32. 🤔 Do you think it is better to use CInt here instead?
There was a problem hiding this comment.
Thanks for the explanation 🙌
No for me Int32 is the good choice, I asked only for know how did you make your choice.
Extends socket lifetime enough to let DispatchSource cancel properly. Also prevents from creating new DispatchSources while other are in the middle of cancelling. Also includes tests (see apple#4854 for test details).
lxbndr
force-pushed
the
readdle/urlsession-dispatchsource-close
branch
from
bab881e
to
f9a54f3
Compare
To avoid issues described in #4791 we have to follow
DispatchSourcecancel procedures in the first place. I.e. we have to prevent underlying socket close untilDispatchSourcecalls a cancel handler.Everything becomes complicated a bit, because we have multiple cases for socket and
DispatchSourcelifecycle. Cancelling Dispatch Source is asynchronous operation (with control points at the start and at the end of the process), and other actions, despite being simple and synchronous, become more complicated due to their dependency on cancel operation. Some of basic cases:easy_handle, as CURL caches connections inmulti_handleThis change aims to extend socket life by tying its lifecycle with boxing object (
_SocketReference). Socket is not closed until_SocketReferenceis alive. WhileDispatchSourcecancel process is ongoing, we're keeping such object, sharing it through a storage with the close socket function. Close socket function implementation marks_SocketReferenceas eligible for closing. If there is no ongoing Dispatch Source cancel, the reference is deinited immediately, effectively closing socket.Also, this change is trying to be as less invasive as possible. Mostly additive, without structural changes. Major work is done by manipulating the state of
_SocketReference. I believe there is better way to handle this, but this would probably require more extended rework of how Dispatch Sources are managed and stored.And the fly in the ointment. This fixes most of crash scenarios on Windows, but not all. I can now say confidently that Dispatch has some flaws related to the socket processing on that platform. During stress testing I observed numerous Dispatch crashes on adding socket handle after it being reused by the system - and that is after graceful and complete cancel of corresponding DispatchSource. Luckily, it appears only on heavy load (test
test_repeatedRequestsStress- marked as expected to fail), but it definitely affects final product and our users.Also, this changeset includes tests from #4854 and timer fixes from #4858 (as it makes everything work more predictable). If this whole PR would be unreliable for some reason, I'd like to merge aforementioned changes separately.