acquire lock in addEdges()

[parasssh]

Fixes DGRAPH-1332

---

This change is 

[martinmr]

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @manishrjain, @parasssh, and @vvbalaji-dgraph)

---

worker/executor.go, line 85 at r1 (raw file):

	defer e.RUnlock()
	for _, ch := range e.predChan {
		close(ch)

is this the close you are referencing in your comment?

Why is the read lock being used here?

---

worker/executor.go, line 126 at r1 (raw file):

	}

	// Lock() in case the channel gets closed from underneath us.

I am still not clear from this comment how locking will prevent us from sending to a closed channel. Can you expand the comment a little bit?

[parasssh]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @manishrjain, @martinmr, and @vvbalaji-dgraph)

---

worker/executor.go, line 85 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

is this the close you are referencing in your comment?

Why is the read lock being used here?

Yes, this close.

Not sure why read lock was acquired here. Presumably

---

worker/executor.go, line 126 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

I am still not clear from this comment how locking will prevent us from sending to a closed channel. Can you expand the comment a little bit?

We will acquire Lock and so have exclusive access. The shutdown() method above will not be able to close the channel since it requires a ReadLock() and will block until we are done here.
The e.getChannel() method creates a channel for that attr if none exists and so the sending on the channel should succeed.

==
May be better way would be to just check here for e.Closer.HasBeenClosed in a non-blocking way and return from here.

[parasssh]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @manishrjain, @martinmr, and @vvbalaji-dgraph)

---

worker/executor.go, line 85 at r1 (raw file):

Previously, parasssh wrote…

Yes, this close.

Not sure why read lock was acquired here. Presumably

The comment was incomplete.

Yes, this close is what I am referring to.
The ReadLock is presumably acquired to close the channel.

[parasssh]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @manishrjain, @martinmr, and @vvbalaji-dgraph)

---

worker/executor.go, line 126 at r1 (raw file):

Previously, parasssh wrote…

We will acquire Lock and so have exclusive access. The shutdown() method above will not be able to close the channel since it requires a ReadLock() and will block until we are done here.
The e.getChannel() method creates a channel for that attr if none exists and so the sending on the channel should succeed.

==
May be better way would be to just check here for e.Closer.HasBeenClosed in a non-blocking way and return from here.

Nevermind. Acquiring Lock here will result in deadlock since getChannel() also needs a lock.
Need to think about a different solution.

[martinmr]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @manishrjain, @martinmr, @parasssh, and @vvbalaji-dgraph)

---

worker/executor.go, line 126 at r1 (raw file):

Previously, parasssh wrote…

Nevermind. Acquiring Lock here will result in deadlock since getChannel() also needs a lock.
Need to think about a different solution.

I think this works if you create and call a version of getChannel that assumes the lock has already been acquired (there are methods to assert this I believe).

[martinmr]

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @manishrjain, @parasssh, and @vvbalaji-dgraph)

---

worker/executor.go, line 90 at r2 (raw file):

// getChannelUnderLock obtains the channel for the given pred. It must be called under e.Lock().
func (e *executor) getChannelUnderLock(pred string) (ch chan *subMutation) {

This is only used once so I don't think you need to rename it. Just the comment should be enough.