Skip to content
This repository has been archived by the owner on Nov 9, 2017. It is now read-only.

Properly release locks in lamina.core.graph.node/propagate #45

Closed
wants to merge 1 commit into from

Conversation

DerGuteMoritz
Copy link
Contributor

Previously, exceptions raised in lamina.core.graph.node/propagate
could lead to a node's lock to not be released properly because
releasing didn't happen in a finally clause. However, using the
with-lock macro is not possible in this case because in some
branches the lock is released before the with-lock body ends. To
allow for this, a with-lock* macro is introduced. If the lock is
released inside its body it will refrain from unlocking it again. To
support this, locked? is added to ILock which checks whether a
lock is currently held. Also, recurring from inside a try form is
not allowed but would be required in this case for traversing a chain
of nodes. Thus the nodes traversal loop is changed to recur from
outside the try form.

I'm not sure if this is the best way to fix the root problem, so feel
free to implement a better solution instead.

Oh, and I threw out some unnecessary do forms from with-lock
and with-exclusive-lock while I was at it :-)

Previously, exceptions raised in `lamina.core.graph.node/propagate`
could lead to a node's lock to not be released properly because
releasing didn't happen in a `finally` clause. However, using the
`with-lock` macro is not possible in this case because in some
branches the lock is released before the `with-lock` body ends. To
allow for this, a `with-lock*` macro is introduced. If the lock is
released inside its body it will refrain from unlocking it again. To
support this, `locked?` is added to `ILock` which checks whether a
lock is currently held.  Also, recurring from inside a `try` form is
not allowed but would be required in this case for traversing a chain
of nodes. Thus the nodes traversal loop is changed to recur from
outside the `try` form.
@DerGuteMoritz
Copy link
Contributor Author

Any news on this one? :-)

@ztellman
Copy link
Owner

Wow, this completely fell off my radar. I'm flying out to the Conj today,
so I can't promise a day's turnaround, but I'll have a fix in very soon.
Sorry about that.

On Wed, Nov 14, 2012 at 6:22 AM, Moritz Heidkamp
notifications@github.comwrote:

Any news on this one? :-)


Reply to this email directly or view it on GitHubhttps://github.com//pull/45#issuecomment-10367497.

@DerGuteMoritz
Copy link
Contributor Author

⏰ Another bi-monthly issue reminder :-)

@ztellman
Copy link
Owner

Hey, I've actually been looking at this, trying to understand where there's the possibility of an exception. I found one in queue.clj, but not in the propagate method in node.clj. Before making the inner loop that much more complicated, I want to understand what's happened.

Just out of curiosity, have you seen this again?

@DerGuteMoritz
Copy link
Contributor Author

I don't think I've seen it again since then. One scenario which I think could cause this is when a thread is stopped while we're in propagate, throwing a java.lang.ThreadDeath and thus leaving the channel in an invalid state. What do you think?

@ztellman
Copy link
Owner

Were you calling Thread.stop()?

@DerGuteMoritz
Copy link
Contributor Author

Not explicitly but I might have been C-c C-cing my SLIME connection at some unfortunate point in time :-)

@ztellman ztellman closed this Dec 2, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants