Issue a warning instead of throwing an exception

[gcasa]

This is a basic solution to issue #316, it is likely not the right one, but I am hoping this PR provides the right forum for discussing what is correct. It is worth noting that macOS does not throw an exception when an unlock fails.

[rfm]

From the point of a developer, the existing code is much better than the Apple-like version. This is dealing with a (potentially critical) bug in the application locking logic, and currently we get the program halted with a log of (assuming stack traces are enabled) exactly where the problem occurred. Throwing all that away in favour of a minimal warning log only makes sense in production code where the developer can't do anything about the problem, and you can hope that continuing might mean the app works (rather than failing in a worse way than just crashing).

So, there are a variety of options:

1. Make the alternatives conditional at runtime using the GSMacOSXCompatible setting, which is provided to turn on OSX behaviors in case where we think they are poor.
2. Make the alternatives conditional at compile time, with the minimal warning if NDEBUG is defined
3. If the aim is to have the program continue after the error, print the stack trace and the name of the lock as part of the warning.

[triplef]

I also strongly agree that this shouldn’t just be a log by default.

Would NSAssert() make sense? That way users can disable these assertions in production code using NS_BLOCK_ASSERTIONS.

That being said, always throwing an exception seems the right thing to do to me, because you will always want to fix your locking logic. @gcasa what’s the underlying issue you are trying to solve here?

[gcasa]

The issue is that I have encountered some production code where there is an attempt to unlock an NSLock which is either unlocked or has never been locked. I have tested this on macOS and there it fails silently which is in contrast to us since we throw an exception.

I have always maintained that we should always recover gracefully whenever possible since that makes it easier for the end users. I like the suggestion of using the GSMacOSXCompatible setting.

[rfm]

we should always recover gracefully whenever possible
That's what the exception lets us do: the higher level code can catch the exception, decide whether graceful recovery is possible, and act accordingly. This is why the current behavior is better than the Apple behavior.

Consider this:
If the code attempts to unlock a lock which is not held, there is a good chance that it's because we failed to lock a region where locking was expected.
If locking was expected, its because we needed to prevent in-memory data structures from being modified by another thread.
So finding the lock not held means theres definitely a chance that in-memory data structures have been corrupted.
If we just log a warning, we are likely to use corrupt data and either
a. have the application behave wrongly or crash or
b. write the corrupt data to disk, so that the problem spreads to other systems

Admittedly I'm biased by the fact that I write server processes which are expected to run 24*7 handling millions of transactions a day (and which are monitored so that crashed processes are automatically restarted and hung/crashed processes raise alarms with sysadmins), so it's particularly important that the software doesn't misbehave and corrupt data.

[gcasa]

I concede what you're saying. It is better to throw the exception because it forces the developer to clean up the locking logic. I think the idea of an option to allow less "strict" behavior is a good idea.

From a devs standpoint the current behavior is desirable as it gives the opportunity to fix the problem. I am, in fact, dealing with that situation now, but there is also a product to release. It's always a balance.

[rfm]

Sure. I'd recommend using GSMacOSXCompatible (default is off) to change it to a warning and improving the warning to (at least) report the lock description (which will include its name if it has one set).

[gcasa]

I have added the default suggested in just unlock. I am not sure if it should be applied elsewhere.

[gcasa]

Apparently, any attempt to access NSUserDefaults from within NSLock fails with a segfault. I believe this is because NSUserDefaults is not initialized at the point where NSLock is called. I will try to use an environment variable instead.

[gcasa]

Same with NSProcessInfo... ;/

[gcasa]

I get a segfault also if I try to do getenv. I don't know what else to try at this point. :/ @rfm Any suggestions?

[gcasa]

I will commit what I have tried so you can see.

[rfm]

On 11 Sep 2023, at 16:00, Gregory Casamento ***@***.***> wrote: I will commit what I have tried so you can see.

I accidentally (lack of git familiarity) put fixes and new test cases in origin/NSLock_fix_issue316 rather than NSLock_fix_issue316 Your basic problem is trying to do stuff in the +initialise method of NSLock ... a test is best done lazily (ie later at the point when it's needed) because there's nothing to be gained by trying to cache information for an event which only ever happens in response to a bug.

[gcasa]

On 11 Sep 2023, at 16:00, Gregory Casamento @.***> wrote:

I will commit what I have tried so you can see.

I accidentally (lack of git familiarity) put fixes and new test cases in origin/NSLock_fix_issue316 rather than NSLock_fix_issue316

Your basic problem is trying to do stuff in the +initialise method of NSLock ... a test is best done lazily (ie later at the point when it's needed) because there's nothing to be gained by trying to cache information for an event which only ever happens in response to a bug.

I actually tried it in init and in the actual unlock method and got a segfault in both places as well. I will try a couple of other things.

[rfm]

On 12 Sep 2023, at 05:31, Gregory Casamento ***@***.***> wrote: On 11 Sep 2023, at 16:00, Gregory Casamento @.***> wrote: I will commit what I have tried so you can see. I accidentally (lack of git familiarity) put fixes and new test cases in origin/NSLock_fix_issue316 rather than NSLock_fix_issue316 Your basic problem is trying to do stuff in the +initialise method of NSLock ... a test is best done lazily (ie later at the point when it's needed) because there's nothing to be gained by trying to cache information for an event which only ever happens in response to a bug. I actually tried it in init and in the actual unlock method and got a segfault in both places as well. I will try a couple of other things. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: ***@***.***>

The code I put in git works (as demonstrated by the testcases I added).

[rfm]

Could it be that there is a problem in the code you are using to test this? It's hard to see how using the standard 'C' getenv() function could mess anything up, so most likely there's a n issue with the test rather than the code being tested.

[gcasa]

Cherry-picked the two changes @rfm made on his alternative branch.

[gcasa]

@rfm let me know if we can merge since this is, essentially, your change. GC

Seems I spoke too soon, one of the tests failed...

[rfm]

On 18 Sep 2023, at 04:37, Gregory Casamento ***@***.***> wrote: @rfm let me know if we can merge since this is, essentially, your change. GC —

Sure ... it works for me, so if it works for you as well then it's good to commit.

[gcasa]

@rfm Reran the test case that failed and it passes. I am merging this based on your previous comment.