On Wed, Jun 28, 2017 at 3:09 PM, Erik Bray wrote:

On Wed, Jun 28, 2017 at 2:26 PM, Nick Coghlan wrote:

...

On 28 June 2017 at 21:40, Erik Bray wrote:

...

My colleague's contention is that given

lock = threading.Lock()

this is simply *wrong*:

lock.acquire() try: do_something() finally: lock.release()

whereas this is okay:

with lock: do_something()

Technically both are slightly racy with respect to async signals (e.g. KeyboardInterrupt), but the with statement form is less exposed to the problem (since it does more of its work in single opcodes).

Nathaniel Smith posted a good write-up of the technical details to the issue tracker based on his work with trio: https://bugs.python.org/issue29988

Interesting; thanks for pointing this out. Part of me felt like this has to have come up before but my searching didn't bring this up somehow (and even then it's only a couple months old itself).

I didn't think about the possible race condition before WITH_CLEANUP_START, but obviously that's a possibility as well. Anyways since this is already acknowledged as a real bug I guess any further followup can happen on the issue tracker.

On second thought, maybe there is a case to made w.r.t. making a documentation change about the semantics of the `with` statement: The old-style syntax cannot make any guarantees about atomicity w.r.t. async events. That is, there's no way syntactically in Python to declare that no exception will be raised between "lock.acquire()" and the setup of the "try/finally" blocks. However, if issue-29988 were *fixed* somehow (and I'm not convinced it can't be fixed in the limited case of `with` statements) then there really would be a major semantic difference of the `with` statement in that it does support this invariant. Then the question is whether that difference be made a requirement of the language (probably too onerous a requirement?), or just a feature of CPython (which should still be documented one way or the other IMO). Erik

On Wed, Jun 28, 2017 at 3:19 PM, Greg Ewing wrote:

Erik Bray wrote:

...

At this point a potentially waiting SIGINT is handled, resulting in KeyboardInterrupt being raised while inside the with statement's suite, and finally block, and hence Lock.__exit__ are entered.

Seems to me this is the behaviour you *want* in this case, otherwise the lock can be acquired and never released. It's disconcerting that it seems to be very difficult to get that behaviour with a pure Python implementation.

I think normally you're right--this is the behavior you would *want*, but not the behavior that's consistent with how Python implements the `with` statement, all else being equal. Though it's still not entirely fair either because if Lock.__enter__ were pure Python somehow, it's possible the exception would be raised either before or after the lock is actually marked as "acquired", whereas in the C implementation acquisition of the lock will always succeed (assuming the lock was free, and no other exceptional conditions) before the signal handler is executed.

...

I think it might be possible to gain more consistency between these cases if pending signals are checked/handled after any direct call to PyCFunction from within the ceval loop.

IMO that would be going in the wrong direction by making the C case just as broken as the Python case.

Instead, I would ask what needs to be done to make this work correctly in the Python case as well as the C case.

You have a point there, but at the same time the Python case, while "broken" insofar as it can lead to broken code, seems correct from the Pythonic perspective. The other possibility would be to actually change the semantics of the `with` statement. Or as you mention below, a way to temporarily mask signals...

I don't think it's even possible to write Python code that does this correctly at the moment. What's needed is a way to temporarily mask delivery of asynchronous exceptions for a region of code, but unless I've missed something, no such facility is currently provided.

What would such a facility look like? One possibility would be to model it on the sigsetmask() system call, so there would be a function such as

mask_async_signals(bool)

that turns delivery of async signals on or off.

However, I don't think that would work. To fix the locking case, what we need to do is mask async signals during the locking operation, and only unmask them once the lock has been acquired. We might write a context manager with an __enter__ method like this:

def __enter__(self): mask_async_signals(True) try: self.acquire() finally: mask_async_signals(False)

But then we have the same problem again -- if a Keyboard Interrupt occurs after mask_async_signals(False) but before __enter__ returns, the lock won't get released.

Exactly.

Another approach would be to provide a context manager such as

async_signals_masked(bool)

Then the whole locking operation could be written as

with async_signals_masked(True): lock.acquire() try: with async_signals_masked(False): # do stuff here finally: lock.release()

Now there's no possibility for a KeyboardInterrupt to be delivered until we're safely inside the body, but we've lost the ability to capture the pattern in the form of a context manager.

The only way out of this I can think of at the moment is to make the above pattern part of the context manager protocol itself. In other words, async exceptions are always masked while the __enter__ and __exit__ methods are executing, and unmasked while the body is executing.

I think so too. That's more or less in line with Nick's idea on njs's issue (https://bugs.python.org/issue29988) of an ATOMIC_UNTIL opcode. That's just one implementation possibility. My question would be to make that a language-level requirement of the context manager protocol, or just something CPython does... Thanks, Erik

On Wed, Jun 28, 2017 at 6:19 AM, Greg Ewing wrote:

Erik Bray wrote:

...

At this point a potentially waiting SIGINT is handled, resulting in KeyboardInterrupt being raised while inside the with statement's suite, and finally block, and hence Lock.__exit__ are entered.

Seems to me this is the behaviour you *want* in this case, otherwise the lock can be acquired and never released. It's disconcerting that it seems to be very difficult to get that behaviour with a pure Python implementation.

I agree :-)

...

I think it might be possible to gain more consistency between these cases if pending signals are checked/handled after any direct call to PyCFunction from within the ceval loop.

IMO that would be going in the wrong direction by making the C case just as broken as the Python case.

Instead, I would ask what needs to be done to make this work correctly in the Python case as well as the C case.

I don't think it's even possible to write Python code that does this correctly at the moment. What's needed is a way to temporarily mask delivery of asynchronous exceptions for a region of code, but unless I've missed something, no such facility is currently provided.

It's *almost* possible in some cases, by installing a specialized signal handler which introspects the stack to see if we're in one of these delicate regions of code. See: https://vorpus.org/blog/control-c-handling-in-python-and-trio/#how-trio-hand... The "almost" is because currently, I have a function decorator that marks certain functions as needing protection against async signals, which works by injecting a magic local variable into the function when it starts. The problem is that this can't be done atomically, so if you have an __exit__ method like: def __exit__(self, *args): # XX _this_function_is_protected = True self.release() then it's possible for a signal to arrive at the point marked "XX", and then your lock never gets released. One solution would be: https://bugs.python.org/issue12857 I've also been considering gross things like keeping a global WeakSet of the code objects for all functions that have been registered for async signal protection. However, trio's solution looks a bit different than what you'd need for a general python program, because the general strategy is that if a signal arrives at a bad moment, we don't delay it (we can't!); instead, we make a note to deliver it later. For an async framework like trio this is fine, and in fact we need the "deliver the signal later" facility anyway, because we need to handle the case where a signal arrives while the event loop is polling for I/O and there isn't any active task to deliver the signal to anyway. For a generic solution in the interpreter, then I agree that it'd probably make more sense to have a way to delay running the signal handler until an appropriate moment.

What would such a facility look like? One possibility would be to model it on the sigsetmask() system call, so there would be a function such as

mask_async_signals(bool)

that turns delivery of async signals on or off.

However, I don't think that would work. To fix the locking case, what we need to do is mask async signals during the locking operation, and only unmask them once the lock has been acquired. We might write a context manager with an __enter__ method like this:

def __enter__(self): mask_async_signals(True) try: self.acquire() finally: mask_async_signals(False)

But then we have the same problem again -- if a Keyboard Interrupt occurs after mask_async_signals(False) but before __enter__ returns, the lock won't get released.

Another approach would be to provide a context manager such as

async_signals_masked(bool)

Then the whole locking operation could be written as

with async_signals_masked(True): lock.acquire() try: with async_signals_masked(False): # do stuff here finally: lock.release()

Now there's no possibility for a KeyboardInterrupt to be delivered until we're safely inside the body, but we've lost the ability to capture the pattern in the form of a context manager.

If async_signals_masked is implemented in C and can be used as a decorator, then you could do: class MyLock: @async_signals_masked(True) def __enter__(self): ... @async_signals_masked(True) def __exit__(self, *exc_info): ... However, there's still a problem: in async code, you can yield out of a async-signals-masked section, either because you have an async context manager: @async_signals_masked(True) async def __aexit__(self, *exc_info): ... or because you're using the context manager directly to protect some delicate code: async def acquire(self): # Make sure KeyboardInterrupt can't leave us with a half-taken lock with async_signals_masked(True): if self._already_held: await self._lock_released_event ... So what should this async_signals_masked state do when we yield out from under it? If it's a thread-local, then the masking state will "leak" into other async function callstacks (or similar for regular generators), which is pretty bad. But it can't be just frame-local data either, because we want the masking state to be inherited by and subroutines we call from inside the masked block. This is why trio uses the stack walking trick: it means that when you use 'yield' to switch callstacks, the async signal masking state gets switched too, automatically and atomically. So maybe a better way would be to do something more like what trio does. For example, we could have a flag on a function frame that says "this frame (and the code in it) should not be interrupted", and then in the bytecode loop when a signal arrives, walk up the call stack to see if any of these flags are set before running the Python-level signal handler. There's some efficiency and complexity questions here, but maybe it's not too bad (signals are only received rarely, and maybe there are some tricks to reduce the overhead).

The only way out of this I can think of at the moment is to make the above pattern part of the context manager protocol itself. In other words, async exceptions are always masked while the __enter__ and __exit__ methods are executing, and unmasked while the body is executing.

This would make me nervous, because context managers are used for all kinds of things, and only some of them involve delicate resource manipulation. Masking async exceptions is a trade-off: if you do it at the wrong place, then you can end up with a program that refuses to respond to control-C, which is pretty frustrating. There are also some rather nasty cases, like I think Popen.__exit__ might block waiting for SIGCHLD to be delivered? And anyway, you still have to solve the problem of how you communicate this state to subroutines called by __(a)enter__ and __(a)exit__, but not let it leak when you yield. Once you solve that I think you have 95% of the machinery you need to make this user-controllable. -n -- Nathaniel J. Smith -- https://vorpus.org

On Wed, Jun 28, 2017 at 9:14 PM, Greg Ewing wrote:

Nathaniel Smith wrote:

...

So what should this async_signals_masked state do when we yield out from under it? If it's a thread-local, then the masking state will "leak" into other async function callstacks (or similar for regular generators), which is pretty bad. But it can't be just frame-local data either, because we want the masking state to be inherited by and subroutines we call from inside the masked block.

That should be easy enough, shouldn't it? When entering a new frame, copy the mask state from the calling frame.

Right, that approach would be semantically equivalent to the walking-the-call-stack approach, just it shifts some of the cost around (it makes signals cheaper by making each function call a tiny bit more expensive).

...

For example, we could have a flag on a function frame that says "this frame (and the code in it) should not be interrupted", and then in the bytecode loop when a signal arrives, walk up the call stack to see if any of these flags are set before running the Python-level signal handler.

That would make it impossible to temporarily unmask async signals in a region where they're masked.

An example of a situation where you might want to do that is in an implementation of lock.acquire(). If the thread needs to block while waiting for the lock to become available, you probably want to allow ctrl-C to interrupt the thread while it's blocked.

So trio actually does allow this kind of nesting -- for any given frame the special flag can be unset, set to True, or set to False, and the signal handler walks up until it finds the first one frame where it's set and uses that. So I guess the equivalent would be two flags, or a little enum field in the frame object, or something like that.

...

This would make me nervous, because context managers are used for all kinds of things, and only some of them involve delicate resource manipulation.

The next step I had in mind was to extend the context manager protocol so that the context manager can indicate whether it wants async signals masked, so it would only happen for things like lock that really need it.

A magic (implemented in C) decorator like @async_signals_masked I think would be the simplest way to do this extension. (Or maybe better call it @no_signal_delivery, because it would have to block all running of signal handlers; the interpreter doesn't know whether a signal handler will raise an exception until it calls it.) -n -- Nathaniel J. Smith -- https://vorpus.org

On Fri, Sep 21, 2018 at 12:58 AM Chris Angelico wrote:

On Fri, Sep 21, 2018 at 8:52 AM Kyle Lahnakoski wrote:

...

Since the java.lang.Thread.stop() "debacle", it has been obvious that stopping code to run other code has been dangerous. KeyboardInterrupt (any interrupt really) is dangerous. Now, we can probably code a solution, but how about we remove the danger:

I suggest we remove interrupts from Python, and make them act more like java.lang.Thread.interrupt(); setting a thread local bit to indicate an interrupt has occurred. Then we can write explicit code to check for that bit, and raise an exception in a safe place if we wish. This can be done with Python code, or convenient places in Python's C source itself. I imagine it would be easier to whitelist where interrupts can raise exceptions, rather than blacklisting where they should not.

The time machine strikes again!

https://docs.python.org/3/c-api/exceptions.html#signal-handling

Although my original post did not explicitly mention PyErr_CheckSignals() and friends, it had already taken that into account and it is not a silver bullet, at least w.r.t. the exact issue I raised, which had to do with the behavior of context managers versus the setup() try: do_thing() finally: cleanup() pattern, and the question of how signals are handled between Python interpreter opcodes. There is a still-open bug on the issue tracker discussing the exact issue in greater details: https://bugs.python.org/issue29988

On Thu, Sep 20, 2018, 3:52 PM Kyle Lahnakoski wrote:

KeyboardInterrupt (any interrupt really) is dangerous. Now, we can probably code a solution, but how about we remove the danger

The other day I accidentally fork-bombed myself with Python os.fork in an infinite loop. Whoops. It seems to me that Python's design philosophy is to make the safe things beautiful and efficient, but not to remove the dangerous things. I'd be supportive of a proposal that makes threading safer without removing capabilities for those that want them.