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

Fixed race condition in RecycleBin. #1828

Merged
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions source/concurrent/RecycleBin.ooc
Original file line number Diff line number Diff line change
Expand Up @@ -10,30 +10,40 @@ use concurrent

RecycleBin: class <T> {
_free: Func (T)
_list: SynchronizedList<T>
_list: VectorList<T>
_size: Int
count ::= this _list count
isFull ::= this count == this _size
isEmpty ::= this _list empty
init: func (=_size, =_free) { this _list = SynchronizedList<T> new(_size) }
_mutex := Mutex new()
Copy link
Contributor

Choose a reason for hiding this comment

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

the count, isFull and isEmpty is not thread safe, change to private?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On second thought, this class should be able to use without a mutex, then those properties should be public anyway.

init: func (=_size, =_free) { this _list = VectorList<T> new(_size) }
free: override func {
this clear()
this _list free()
(this _free as Closure) free(Owner Receiver)
this _mutex free()
super()
}
add: func (object: T) {
this _mutex lock()
Copy link
Contributor

Choose a reason for hiding this comment

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

this _mutex with(...) here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rock didn't accept that unfortunately

if (this isFull)
this _free(this _list remove(0))
this _list add(object)
this _mutex unlock()
}
search: func (matches: Func (T) -> Bool) -> T {
this _mutex lock()
Copy link
Contributor

Choose a reason for hiding this comment

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

@sebastianbaginski Somewhere you changed to put result: T = null inside the lock, is that good as a general rule or was it a special case? I don't remember the specifics.

Copy link

Choose a reason for hiding this comment

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

It was here #1821 (comment)
Yes, it does matter for generics, because this line result: T = null can trigger the "constructor" of the Pointer_Class due to lazy initialization. This constructor can then modify a static bool flag.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. Looks like it's being fixed, but won't make it to us any time soon I'm afraid. ooc-lang/rock#1004
In the mean time, @emilwestergren - move line 35 below line 36.

index := this _list search(matches)
result: T = null
(index > -1) ? this _list remove(index) : result
if (index > -1)
result = this _list remove(index)
this _mutex unlock()
result
}
clear: func {
this _mutex lock()
while (!this isEmpty)
this _free(this _list remove())
this _mutex unlock()
}
}