[8.x] Ensure event mutex is always removed

[inxilpro]

This is a fix for #39452

• Added a new removeMutex method (which already exists on CallbackEvent) that only clears the mutex if necessary
• For commands running foreground, we wrap everything in a try/finally to call removeMutex
• For commands running the the background, we wrap the before callbacks in a try/catch that re-throws any exceptions after first calling removeMutex. Then, we call removeMutex in callAfterCallbacksWithExitCode after the background job completes.

[taylorotwell]

The withoutOverlapping command still releases the mutex. Could this not create a race condition where the withoutOverlapping callback releases the mutex, then a new scheduled command grabs a new mutex, but then the removeMutex method is called and deletes the newly obtained mutex for the other command?

[inxilpro]

Could this not create a race condition

Oh shoot. Good point. I decided to leave that in there to change as few things possible—I have a 9.x version of the PR that I'm going to push up that cleans things up a little more but adds a few new methods to Event that could be theoretically backwards-incompatible. But yeah, I'll remove the after() callback from this, too.

[inxilpro]

@taylorotwell It kinda looks like CallbackEvent has the same issue:

framework/src/Illuminate/Console/Scheduling/CallbackEvent.php

Lines 67 to 73 in cb46639

	$pid = getmypid();
	register_shutdown_function(function () use ($pid) {
	if ($pid === getmypid()) {
	$this->removeMutex();
	}
	});

I'm having trouble understanding why CallbackEvent handles mutex's so differently than normal Event objects. It seems to me that the register_shutdown_function call either should exist on both objects or should be removed. I'm working on a 9.x version of this that includes a little refactoring, and I'd like to bring the implementations of the two closer together. Do you have any more context about why they're so different right now?

[taylorotwell]

I don't know why but I would be very cautious changing things we don't fully understand. ALWAYS leads to breaking changes.

[taylorotwell]

@inxilpro, so there is some remaining issue that prevents this particular PR from being merged? Are you sure register_shutdown_function events won't be called on an exception?

[inxilpro]

I think that this can be merged as-is. If there's an issue with register_shutdown_function it was already there. It's a pretty edge-case race condition, so I'm not surprised it hasn't come up.

But register_shutdown_function callbacks are called if an exception is thrown. That's the issue. The mutex is removed inside a finally block and also during the shutdown function so theoretically the same race condition you identified could happen for CallbackEvent execution as well:

1. Callback A starts executing
2. Callback A triggers an exception, causing the removeMutex call in the finally block. At the exact same moment in time, callback B starts and is able to run because the mutex was removed.
3. The register_shutdown_function callback from Callback A is executed, removing the mutex for callback B.
4. Callback C starts and is able to run because the mutex was removed twice by callback A.

It's very very unlikely for that sequence to happen, but I think it's theoretically possible…