Skip to content

Weak mutex locking macros#5879

Merged
duke8253 merged 1 commit intoapache:masterfrom
duke8253:master-check_null
Oct 3, 2019
Merged

Weak mutex locking macros#5879
duke8253 merged 1 commit intoapache:masterfrom
duke8253:master-check_null

Conversation

@duke8253
Copy link
Contributor

This deals with the problem brought up in the discussions under PRs #5855 and #5857. And also makes the change such that #4926 still works as intended.

@duke8253 duke8253 added the Core label Aug 26, 2019
@duke8253 duke8253 added this to the 9.1.0 milestone Aug 26, 2019
@duke8253 duke8253 self-assigned this Aug 26, 2019
@bryancall
Copy link
Contributor

[approve ci autest]

@scw00
Copy link
Member

scw00 commented Aug 27, 2019

Should this pr revert ? #5857

@duke8253
Copy link
Contributor Author

[approve ci autest]

@bryancall
Copy link
Contributor

@scw00 Unfortunately, no. These changes add 2 extra conditionals on the normal SCOPED_MUTEX_LOCK use case (the ink_release_assert on the mutex pointer in the macro and then again on conditional in the constructor).

@bryancall bryancall modified the milestones: 9.1.0, 10.0.0 Aug 27, 2019
@scw00
Copy link
Member

scw00 commented Aug 28, 2019

It seems this pr is duplicated with #5857. So why do we still need #5857 .

@SolidWallOfCode
Copy link
Member

@scw00 I presume this will effectively revert #5857, once we are clear on the best way to implement.

@SolidWallOfCode
Copy link
Member

@duke8253 and I discussed the plan I put on the chat, and he's going to redo this PR using that design. Basically, create a NullMutex class with no state that always succeeds in locking, create a global singleton, and then pass the address of that to the MutexLock classes in the nullptr case for the WEAK... variants. The previously existing macros and the MutexLock classes are reverted to their previous behavior.

@SolidWallOfCode
Copy link
Member

Well, that turned out to not work. Sigh. @duke8253's trying something else now.

@duke8253
Copy link
Contributor Author

Not sure why it's failing on systems other than Ubuntu, I'll get some vms to test.

@duke8253 duke8253 force-pushed the master-check_null branch 4 times, most recently from e2f899f to 4b6e25d Compare September 4, 2019 21:19
@duke8253
Copy link
Contributor Author

duke8253 commented Sep 5, 2019

[approve ci clang-analyzer]

@duke8253 duke8253 requested a review from bryancall October 3, 2019 18:52
@bryancall
Copy link
Contributor

This PR is to fix this core dump (supplied by @duke8253):

[ 0 ] traffic_server      Mutex_lock                            ( I_Lock.h:295 )
[ 1 ] traffic_server      MutexLock                             ( I_Lock.h:363 )
[ 2 ] traffic_server      AutoStopCont::mainEvent(int, Event*)  ( traffic_server.cc:233 )
[ 3 ] traffic_server      EThread::process_event(Event*, int)   ( I_Continuation.h:167 )
[ 4 ] traffic_server      EThread::execute_regular()            ( UnixEThread.cc:249 )
[ 5 ] traffic_server      execute                               ( UnixEThread.cc:338 )
[ 6 ] traffic_server      EThread::execute()                    ( UnixEThread.cc:316 )
[ 7 ] traffic_server      spawn_thread_internal                 ( Thread.cc:92 )
[ 8 ] libpthread-2.17.so  start_thread                          

Copy link
Contributor

@bryancall bryancall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good

@duke8253 duke8253 merged commit f0d82b7 into apache:master Oct 3, 2019
@bryancall
Copy link
Contributor

Cherry picked to the 9.0.x branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants