Skip to content

Conversation

@niaow
Copy link
Member

@niaow niaow commented Jan 27, 2020

This is based on top of #828 and adds full support for the sync package, implementing blocking by using the internal/task package.

@niaow niaow force-pushed the internalcorosync branch 2 times, most recently from 0028eef to fc2b5d5 Compare January 27, 2020 23:34
@aykevl aykevl mentioned this pull request Feb 20, 2020
@niaow niaow force-pushed the internalcorosync branch from 3db2520 to c462454 Compare March 17, 2020 14:51
@niaow niaow marked this pull request as ready for review March 17, 2020 15:28
@niaow
Copy link
Member Author

niaow commented Mar 17, 2020

I have implemented most of sync, and I am not quite sure how to test this kind of stuff.

@deadprogram
Copy link
Member

Do you mean usage testing? If so then I can give this a try in the mqtt code we have that could really use a mutex.

@aykevl
Copy link
Member

aykevl commented Mar 18, 2020

Yeah not sure how to best test this either. The happy flow should already be mostly tested in testdata/channel.go, and that is the only thing that's easy to test. It's hard to prove the non-existence of a concurrency bug. Some more real-world testing would definitely help build confidence, though!

Copy link
Member

@aykevl aykevl left a comment

Choose a reason for hiding this comment

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

I have some questions about the sync.Cond design. Maybe I misunderstood something but it doesn't seem sound right now. See my comments below.

The other commits look good to me (although I haven't tested them).

@niaow niaow force-pushed the internalcorosync branch from 9902961 to 49ea7d6 Compare March 26, 2020 13:31
niaow added 6 commits March 26, 2020 09:31
While adding some code to clear the Next field when popping from a task stack for safety reasons, the clear was placed outside of a nil pointer check.
As a result, (*internal/task.Stack).Pop panicked when the Stack is empty.
@niaow
Copy link
Member Author

niaow commented Mar 26, 2020

CI had failed because of the issue described in #977, so I rebased on dev.

@aykevl
Copy link
Member

aykevl commented Mar 27, 2020

Looks good to me. However, some more testing would be good to have. @deadprogram you mentioned you might have some code with which you could test this?

Also, you could add some tests to ./testdata/coroutines.go to check that at least the happy path works. Something along the lines of:

// one goroutine
var l sync.Mutex
go unlocker()
l.Lock() // should suceed
println("mutex locked for the first time")
l.Lock() // should block until unlocked by the other goroutine
println("mutex locked for the second time")

// another goroutine
func unlocker() {
    l.Unlock()
    println("mutex unlocked...")
}

@niaow
Copy link
Member Author

niaow commented Mar 27, 2020

@aykevl I added a test as requested.

@niaow niaow added this to the v0.14 milestone Apr 13, 2020
@niaow niaow requested a review from aykevl April 26, 2020 15:13
@aykevl aykevl merged commit b481519 into tinygo-org:dev May 8, 2020
@aykevl
Copy link
Member

aykevl commented May 8, 2020

Thanks a lot!

I have merged this without squasing because there were various commits that really were different features, although there were also some that might have been better squashed together with some other commit (such as the last one). I didn't want to lose that history.

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.

3 participants