search

lukechampine / us

An alternative interface to Sia
MIT License
55 stars 4 forks source link

HostSet.Close forgets releasing locks #166

Closed jkawamoto closed 1 year ago

jkawamoto commented 3 years ago

lh.mu needs to be released.

https://github.com/lukechampine/us/blob/36e1081712379a6af2b3f49ca4aadfb1b1786625/renter/renterutil/hostset.go#L102-L112

lukechampine commented 3 years ago

This was intentional. It prevents the HostSet from being used after Close is called. Is this causing problems?

jkawamoto commented 3 years ago

Yes. It blocks the goroutine in acquireCtx.

jkawamoto commented 3 years ago

It's also not a good idea to block goroutines to prevent the HostSet from being used after Close is called. Users are hard to find the problem. It should return a proper error instead.

lukechampine commented 3 years ago

I wrote it this way to catch developer errors, and it did: calling acquire after calling Closeshould be a developer error. I agree that deadlocking isn't ideal, but it was very simple to implement and was guaranteed to work. Anyway, now we have acquireCtx, whose goroutine outlives its parent, so we can't treat it as a developer error anymore. Maybe eventually I'll fix acquireCtx and then I can turn this into a panic instead.

jkawamoto commented 3 years ago

Even if it's a developer error, how do people know there is an error? They need a core dump to find there is a goroutine that tries to acquire a session after HostSet is closed. Such a goroutine never returns, and you might expect people can find it. However, in reality, if a goroutine doesn't return for a long time, the upper context times out or gets cancelled, and the whole application keeps running. As a result, people cannot know there is a goroutine waiting for a lock.

panic isn't also a good idea. panic should be used only for unrecoverable run-time errors: https://golang.org/doc/effective_go#panic. If this is a developer error, it simply should return an error. They say

Real library functions should avoid panic. If the problem can be masked or worked around, it's always better to let things continue to run rather than taking down the whole program.

Actually, how can we avoid acquiring sessions after the host set is closed? Ideally, we need to check if all corresponding goroutines finish before closing the host set. But, it's not practical. For example, we don't know when the goroutine in acquireCtx ends. Another solution would be to add a reference counter and close the host set only if the counter becomes zero. But, such a counter should be implemented in HostSet if necessary.

I think simply returning an error is enough in this case.