Prevent execution of tasks on an already shutdown EL #1395
Conversation
|
Can one of the admins verify this patch? |
9 similar comments
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
| public var safeToExecute: Bool { | ||
| if self.inEventLoop { | ||
| return true | ||
| } |
There was a problem hiding this comment.
I don't think this escape hatch is right. We can be in the event loop but at a stage where further enqueues of work will fail. In fact, the obvious situation where that happens is when the event loop is shut down. At that point we'll dequeue all pending tasks that have not yet executed and fail them. Failing those tasks dispatches callbacks, and those callbacks may try to call back around to add work to the task queue. That's a bad pattern, and we should catch it, but we won't if this hook is here.
I think we only want the external state check.
There was a problem hiding this comment.
Actually, I think we can do better.
The MultithreadedEventLoopGroup currently uses a _scheduledTasks PriorityQueue to store pending work items. This queue is guarded by _tasksLock. I propose that we wrap the PriorityQueue in a data structure that allows us to "scribble" it: to mark it permanently as unusable once we're sure we aren't going to touch it again.
We can do this just by making the PriorityQueue optional, I think, and force unwrapping it whenever we try to modify it. That will allow us to catch the case where tasks are enqueued too early. It doesn't give a good error message, but that might be fine.
If we really want a good error message, a custom enum that is basically an optional with the method we want would also work.
This avoids the need for a new computed property, and also avoids synchronisation concerns: we don't have to worry about communication between two locks. It also allows us to use the point in the actual code where we know no further tasks can execute (the defer block at the top of func run()) to guarantee that outcome.
There was a problem hiding this comment.
Ah, makes sense! Looking into it.
There was a problem hiding this comment.
Ok, pushed up a new approach with _scheduledTasks as an optional
There was a problem hiding this comment.
@weissi @Lukasa I think you could break this task into 3 parts:
- identify the minimum set of places where a check needs to be made before executing
- identify what to query as part of the check
- based on step 2, possibly write some data on close that can correctly fulfill the query
Since tasks can be enqueued from inside and outside the thread, I don't think there is any solution to step 1 that would allow us to use just internalState or just externalState safely.
My original approach branched on inEventLoop (though as @Lukasa pointed out I missed a test case), alternatively, current approach adds a bit of info that can be queried from tasksLock and thus all cases from step 1.
There was a problem hiding this comment.
@realdoug hmm, I'm somewhat lost here. I'm pretty sure we can find out what state we're in wherever we are because:
- if we're on the
EventLoop, we can learn that we won't be processing new tasks if theinternalStateisnoLongerRunning - if we're off the
EventLoop, then we should be in.closing/.closedwhen we're shutting down
If we need another internal (or external) state, for example .aboutToExitThread or something, then I'm happy for you to add such a state. So for example at the very point where you currently set self._scheduledTasks = nil you could
assert(self.state == .noLongerRunning, "illegal state: \(self.state)")
self.state = . aboutToExitThread
Then in schedule0 you could
if self.inEventLoop {
switch self.internalState {
...
}
} else {
switch self.externalState {
...
}
}and in those switches you can then do the appropriate checks depending on whether we're inside or outside the eventloop.
Does that make sense?
There was a problem hiding this comment.
Sorry, I think I got excited and wrote too much :) My only point was that there are two solutions: an if statement or an optional. Using just one of the two states won't work.
I pushed up a new version w/ an if statement inside _schedule0 based on your description. Let me know what you think.
Awesome! Happy to make updates if needed. |
| @@ -56,7 +56,7 @@ internal final class SelectableEventLoop: EventLoop { | |||
| /* private but tests */ internal let _selector: NIO.Selector<NIORegistration> | |||
| private let thread: NIOThread | |||
| @usableFromInline | |||
| internal var _scheduledTasks = PriorityQueue<ScheduledTask>(ascending: true) | |||
| internal var _scheduledTasks: PriorityQueue<ScheduledTask>? = PriorityQueue<ScheduledTask>(ascending: true) | |||
There was a problem hiding this comment.
@realdoug one question here: Why do we make this optional instead of say adding a state to the InternalState?
There was a problem hiding this comment.
That was my suggestion: #1395 (comment)
| // reserve the correct capacity so we don't need to realloc later on. | ||
| scheduledTasksCopy.reserveCapacity(_scheduledTasks.count) | ||
| while let sched = _scheduledTasks.pop() { | ||
| scheduledTasksCopy.reserveCapacity(_scheduledTasks!.count) |
There was a problem hiding this comment.
nit: self. missing in front of _scheduledTasks twice (this is some old code so it was missing previously too)
| public var safeToExecute: Bool { | ||
| if self.inEventLoop { | ||
| return true | ||
| } |
There was a problem hiding this comment.
@realdoug hmm, I'm somewhat lost here. I'm pretty sure we can find out what state we're in wherever we are because:
- if we're on the
EventLoop, we can learn that we won't be processing new tasks if theinternalStateisnoLongerRunning - if we're off the
EventLoop, then we should be in.closing/.closedwhen we're shutting down
If we need another internal (or external) state, for example .aboutToExitThread or something, then I'm happy for you to add such a state. So for example at the very point where you currently set self._scheduledTasks = nil you could
assert(self.state == .noLongerRunning, "illegal state: \(self.state)")
self.state = . aboutToExitThread
Then in schedule0 you could
if self.inEventLoop {
switch self.internalState {
...
}
} else {
switch self.externalState {
...
}
}and in those switches you can then do the appropriate checks depending on whether we're inside or outside the eventloop.
Does that make sense?
| switch self.internalState { | ||
| case .exitingThread: | ||
| return false | ||
| default: |
There was a problem hiding this comment.
could you change this from default: to enumerating the cases explicitly like case .running..., .running..., .noLongerRunning? It's important that we get the compiler errors if we add a case.
| @usableFromInline | ||
| internal var _validExternalStateToScheduleTasks: Bool { | ||
| switch self.externalState { | ||
| case .open, .closing: |
There was a problem hiding this comment.
could you change this from default: to enumerating the cases explicitly like case .running..., .running..., .noLongerRunning? It's important that we get the compiler errors if we add a case.
There was a problem hiding this comment.
@weissi ok done & done
|
@swift-nio-bot test this please |
|
CC @Lukasa |
|
@swift-nio-bot test this please |
Motivation: Since apple#1395, we crash when something is scheduled to an already shutdown EventLoop. That's a good intent (apple#1383) but we feel it's a little too risky right now. Modifications: Print an ERROR instead of just crashing. Result: Fewer crashes.
Motivation: Since apple#1395, we crash when something is scheduled to an already shutdown EventLoop. That's a good intent (apple#1383) but we feel it's a little too risky right now. Modifications: Print an ERROR instead of just crashing. Result: Fewer crashes.
Motivation: Since apple#1395, we crash when something is scheduled to an already shutdown EventLoop. That's a good intent (apple#1383) but we feel it's a little too risky right now. Modifications: Print an ERROR instead of just crashing. Result: Fewer crashes.
Resolve #1383 ; crash if a task is attempted on a closed Event Loop
One note: I created some test cases here, but wasn't able to figure out how to integrate them into IntegrationTests but perhaps you can point me in the right direction and I'll add them.
Modifications:
Make
SelectableEventLoop._scheduledTasksan optional, set it to null when shutting down and throw a precondition failure if task enqueuing is attempted while it is null (i.e. after shutdown).Result:
see test cases