Fluid channels should match the semantics of Go Channels #9265
Conversation
| // We cannot do the data transfer because | ||
| // this QueueMessage was added by Select | ||
| // and some other case was executed. | ||
| // So call the Send function again. | ||
| // We do not care about notifying other | ||
| // because they would have been notified | ||
| // by the executed select case. | ||
| return send_return(Send(item)); | ||
| Send(item); |
There was a problem hiding this comment.
Do you need to "lock.unlock();" here?
There was a problem hiding this comment.
I think you are right, it makes sense to release the lock there. With the new semantics if the nested method call leads to an exception then the outer lock will be held forever.
| // TODO(abhinavarora) Should panic on closed channel | ||
| return send_return(!m->chan_closed); | ||
| if (m->chan_closed) { | ||
| send_return(); |
There was a problem hiding this comment.
Should unlock before throwing exception
| send_return(); | ||
| PADDLE_THROW("Cannot send on closed channel"); | ||
| } | ||
| send_return(); |
There was a problem hiding this comment.
Good catch, the lock needs to be unlocked here. Thank you for pointing this out.
| @@ -118,15 +117,15 @@ bool ChannelImpl<T>::CanReceive() { | |||
| } | |||
|
|
|||
| template <typename T> | |||
| bool ChannelImpl<T>::Send(T *item) { | |||
| void ChannelImpl<T>::Send(T *item) { | |||
| send_ctr++; | |||
| std::unique_lock<std::recursive_mutex> lock{mu_}; | |||
There was a problem hiding this comment.
Do you need to explicitly lock after constructor? lock->lock() ?
There was a problem hiding this comment.
No we don't need to do that. The unique lock constructor automatically does that.
paddle/fluid/framework/channel.h
Outdated
| bool Send(T* data) { | ||
| if (!IsInitialized()) return false; | ||
| void Send(T* data) { | ||
| PADDLE_ENFORCE_EQ(IsInitialized(), true); |
There was a problem hiding this comment.
Maybe It is better that adding exception information for PADDLE_ENFORCE_EQ,
e.g.
PADDLE_ENFORCE_EQ(IsInitialized(), true, "The channel hasn't been initialized.");
Fixes #8813