Skip to content
This repository has been archived by the owner on Oct 26, 2021. It is now read-only.

Fix wrong initial semaphore state #4

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aleksei-timofeyev
Copy link

If O_CREAT is specified, and a semaphore with the given name already exists, then mode and value are ignored. The value should be set to 1 in over case.

If O_CREAT is specified, and a semaphore with the given name already exists, then mode and value are ignored. The value should be set to 1 in over case.
@aleksei-timofeyev
Copy link
Author

@ihuerner please review.

@mdanilov
Copy link
Contributor

mdanilov commented Mar 7, 2018

@jeremiah We faced with this issue (rare occurence) on our project and think it's critical because if it is happened it may affect the whole functionality of persistence because it is the common component. In our case persistence administrator was not able to install the database.

@jeremiah jeremiah requested a review from ihuerner March 7, 2018 20:43
@jeremiah jeremiah requested a review from guysagnes March 7, 2018 20:45
@ihuerner
Copy link

l don't get the problem.
As you said, if a semaphore with the given name already exists, then mode and value are ignored.
This is the case here, why should I modify the ignored parameters?

@mdanilov
Copy link
Contributor

@ihuerner Actually we had a case when semaphore does not exist, and in this case it was created with value 0 and then it was stuck on sem wait and failed due to timeout.

@@ -258,7 +258,7 @@ sint_t pers_lldb_open(str_t const* dbPathname, pers_lldb_purpose_e ePurpose, boo
DLT_LOG(persComLldbDLTCtx, DLT_LOG_INFO,
DLT_STRING(LT_HDR); DLT_STRING(__FUNCTION__); DLT_STRING(": semaphore already exists: "); DLT_STRING(strerror(error)));
//try to open existing semaphore
pLldbHandler->kissDb.kdbSem = sem_open(pLldbHandler->kissDb.semName, O_CREAT, 0644, 0);
pLldbHandler->kissDb.kdbSem = sem_open(pLldbHandler->kissDb.semName, O_CREAT, 0644, 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Despite of comment above (try to open existing semaphore), sometimes the semaphore does not exist when set_open is called, in this case it will be created with incorrect 0 value.

Choose a reason for hiding this comment

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

If the tests are still running successfully, I think we can accecpt the suggested modification.
Did you run the tests?

@gunnarx
Copy link
Member

gunnarx commented Sep 12, 2018

@mdanilov Did you run the tests?

@gunnarx
Copy link
Member

gunnarx commented Jul 19, 2019

@martin-ejdestig Have you run into this issue at all? What do you think, should this change be made?

@martin-ejdestig
Copy link
Contributor

martin-ejdestig commented Jul 19, 2019

@martin-ejdestig Have you run into this issue at all?

Not yet. We (PELUX team) are basically trying to get persistence-storage working at all so that we can evaluate it.

What do you think, should this change be made?

I do not know. But... is the problem perhaps that there is a race condition here? It was a while since I used POSIX semaphores but if the semaphore exists when the first sem_open() with O_EXCL is called and then does not exist when the second call is made, it must have been destroyed with sem_unlink() from somewhere else between these calls.

So... I guess it is correct to have a 1 in the second sem_open() call since we always want the initial value to be 1. And the value is ignored if the semaphore already exists.

But then... if that is correct... why is this just not a single sem_open() call without O_EXCL instead of two? What is gained by passing O_EXCL the first time and leaving it out the second time? If something is gained by doing it this way... it can not be relied on... since the semaphore can apparently be destroyed between the two sem_open() calls.... it is racey.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants