Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure that status lockfile is closed before trying to release work #320

Closed
wants to merge 1 commit into from

Conversation

@chrismeyersfsu
Copy link
Member

I can't put my finger on it. But this feels like the wrong way to solve this.

@@ -366,8 +368,14 @@ func (bwu *BaseWorkUnit) UnredactedStatus() *StatusFileData {
func (bwu *BaseWorkUnit) Release(force bool) error {
bwu.statusLock.Lock()
defer bwu.statusLock.Unlock()
if bwu.status.LockFile != nil {
// There seems to be a race condition with the `defer`s that
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're right here, having already passed the != nil check, and some other goroutine or thread gains control and sets bwu.status.LockFile to nil, then the following call to Close() will panic because it is trying to call a function on a nil pointer.

Copy link
Member

Choose a reason for hiding this comment

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

eek good point. My first instinct is that we ought to make more use of the statusLock mutex. There are some methods that are reading and writing to status file without getting a lock. monitorLocalStatus calls Load, which grabs status file lock, without getting the mutex lock, for example.

Copy link
Contributor

Choose a reason for hiding this comment

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

I used way too many mutex locks in Receptor. A better and more Go-idiomatic way to do this would be to have a single goroutine that does all the status file changes, with channels for other processes to send updates or receive current data.

Copy link
Contributor

Choose a reason for hiding this comment

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

I remember there was a lot of subtlety to this, that I spent a lot of time on it, and that the absence of mutex locks in the status file updates is not just an accident - I had something in mind. But looking at it now, it seems pretty reasonable that you should hold a read lock on BaseWorkUnit.statusLock while performing a file update, or a full lock if you're reading back from the file to memory (ie, changing the in-memory data).

@shanemcd
Copy link
Member Author

Replaced by #321

@shanemcd shanemcd closed this May 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants