Wip Pr for thread safety

[Rohith-Raju]

#233

[gavv]

I suggest the following implementation:

• make c.failure a pointer
• in chain.fail set this pointer
• in chain.leave, in the block under a lock, read c.context, c.handler, c.failure and c.flags into local variables
• in chain.leave, after releasing the lock, do this:
  • if flagFailed is set, and failure is non nil, report it to handler
  • if flagFailed or flagFailedChildren is set, propagate failure to parents (that's what currently is done if reportFailure boolean is true)
  • if none of these flags are set, report success to handler
• boolean flags reportSuccess and reportFailure can be removed
• we need a test for chain, which uses mock AssertionHandler and checks that fail does not report failure and instead leave reports it

[coveralls]

Coverage: 94.925% (+0.02%) from 94.903% when pulling f5c024e on Rohith-Raju:dummy_pr into 015196a on gavv:master.

[Rohith-Raju]

• we need a test for chain, which uses mock AssertionHandler and checks that fail does not report failure and instead leave reports it

This is for the coverage right?.. Ill do it

[gavv]

• we need a test for chain, which uses mock AssertionHandler and checks that fail does not report failure and instead leave reports it

This is for the coverage right?.. Ill do it

This is important part of contract - other code will rely on fact that AssertionHandler is called in leave(), not in fail(). So it's important to fix this contract in test.

[Rohith-Raju]

This is important part of contract - other code will rely on fact that AssertionHandler is called in leave(), not in fail(). So it's important to fix this contract in test.

Yes, this makes sense

[Rohith-Raju]

@gavv
So by doing these changes in c83c252 where I went wrong was that I reported handler when parent chain was locked and that blocked me out. Now f5c024e we are first releasing the lock and then reporting which is allowed. Is this analysis right? am I missing something?

[gavv]

@Rohith-Raju

The current code is still broken and looks like you ignored explanations (in chat) and suggestions (here in PR) or didn't read them carefully...

You're writing c.failure outside of the lock. You're then reading several chain fields into local vars outside of the lock too. This is a data race. Note: I'm talking about chain's lock here, not about parent's lock. Also, as I mentioned, it's better to get rid of reportSuccess and reportFailure. Also, as I also mentioned, PR currently contains some unrelated renames, it's make reviewing a bit harder, please rollback that. Yeah, also this is not a correct way to check if flag is set: flags&flagFailed == 1.

[Rohith-Raju]

ok will take a good look into it