Repository: tinkerpop Updated Branches: refs/heads/master 91d9c623b -> 9913d64cd
Fixed a leak in a Netty ReferenceCounted resource in Gremlin Server. If a Frame contained a Netty Bytebuf which is ReferenceCounted and that Frame did not write downstream because of an exception (perhaps during a transactional commit seemed like the likely place that would happen) then the Bytebuf would not get cleaned up properly and Netty would issue warnings. TINKERPOP-1375 CTR Project: http://git-wip-us.apache.org/repos/asf/tinkerpop/repo Commit: http://git-wip-us.apache.org/repos/asf/tinkerpop/commit/9913d64c Tree: http://git-wip-us.apache.org/repos/asf/tinkerpop/tree/9913d64c Diff: http://git-wip-us.apache.org/repos/asf/tinkerpop/diff/9913d64c Branch: refs/heads/master Commit: 9913d64cd1f9050b3d047b59dcea34ee8507d2dd Parents: 91d9c62 Author: Stephen Mallette <[email protected]> Authored: Mon Jul 25 11:52:46 2016 -0400 Committer: Stephen Mallette <[email protected]> Committed: Mon Jul 25 11:52:46 2016 -0400 ---------------------------------------------------------------------- CHANGELOG.asciidoc | 1 + .../tinkerpop/gremlin/server/handler/Frame.java | 11 +++++ .../gremlin/server/op/AbstractOpProcessor.java | 44 +++++++++++++------- 3 files changed, 40 insertions(+), 16 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/9913d64c/CHANGELOG.asciidoc ---------------------------------------------------------------------- diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index a5310ca..eb25c73 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -26,6 +26,7 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima TinkerPop 3.2.2 (NOT OFFICIALLY RELEASED YET) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +* Fixed a potential leak of a `ReferenceCounted` resource in Gremlin Server. * Added class registrations for `Map.Entry` implementations to `GryoMapper`. [[release-3-2-1]] http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/9913d64c/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/Frame.java ---------------------------------------------------------------------- diff --git a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/Frame.java b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/Frame.java index e6a616f..76d772e 100644 --- a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/Frame.java +++ b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/Frame.java @@ -18,6 +18,8 @@ */ package org.apache.tinkerpop.gremlin.server.handler; +import io.netty.util.ReferenceCounted; + /** * A holder for a {@code String} or {@code ByteBuf} that represents a message to be written back to the requesting * client. @@ -34,4 +36,13 @@ public class Frame { public Object getMsg() { return msg; } + + /** + * If the object contained in the frame is {@code ReferenceCounted} then it may need to be released or else + * Netty will generate warnings that counted resources are leaking. + */ + public void tryRelease() { + if (msg instanceof ReferenceCounted) + ((ReferenceCounted) msg).release(); + } } http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/9913d64c/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/op/AbstractOpProcessor.java ---------------------------------------------------------------------- diff --git a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/op/AbstractOpProcessor.java b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/op/AbstractOpProcessor.java index 29861f2..4b10bbe 100644 --- a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/op/AbstractOpProcessor.java +++ b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/op/AbstractOpProcessor.java @@ -128,31 +128,43 @@ public abstract class AbstractOpProcessor implements OpProcessor { // serialize here because in sessionless requests the serialization must occur in the same // thread as the eval. as eval occurs in the GremlinExecutor there's no way to get back to the // thread that processed the eval of the script so, we have to push serialization down into that - Frame frame; + Frame frame = null; try { frame = makeFrame(ctx, msg, serializer, useBinary, aggregate, code); } catch (Exception ex) { + // a frame may use a Bytebuf which is a countable release - if it does not get written + // downstream it needs to be released here + if (frame != null) frame.tryRelease(); + // exception is handled in makeFrame() - serialization error gets written back to driver // at that point if (manageTransactions) attemptRollback(msg, context.getGraphManager(), settings.strictTransactionManagement); break; } - // only need to reset the aggregation list if there's more stuff to write - if (itty.hasNext()) - aggregate = new ArrayList<>(resultIterationBatchSize); - else { - // iteration and serialization are both complete which means this finished successfully. note that - // errors internal to script eval or timeout will rollback given GremlinServer's global configurations. - // local errors will get rolledback below because the exceptions aren't thrown in those cases to be - // caught by the GremlinExecutor for global rollback logic. this only needs to be committed if - // there are no more items to iterate and serialization is complete - if (managedTransactionsForRequest) attemptCommit(msg, context.getGraphManager(), settings.strictTransactionManagement); - - // exit the result iteration loop as there are no more results left. using this external control - // because of the above commit. some graphs may open a new transaction on the call to - // hasNext() - hasMore = false; + try { + // only need to reset the aggregation list if there's more stuff to write + if (itty.hasNext()) + aggregate = new ArrayList<>(resultIterationBatchSize); + else { + // iteration and serialization are both complete which means this finished successfully. note that + // errors internal to script eval or timeout will rollback given GremlinServer's global configurations. + // local errors will get rolledback below because the exceptions aren't thrown in those cases to be + // caught by the GremlinExecutor for global rollback logic. this only needs to be committed if + // there are no more items to iterate and serialization is complete + if (managedTransactionsForRequest) + attemptCommit(msg, context.getGraphManager(), settings.strictTransactionManagement); + + // exit the result iteration loop as there are no more results left. using this external control + // because of the above commit. some graphs may open a new transaction on the call to + // hasNext() + hasMore = false; + } + } catch (Exception ex) { + // a frame may use a Bytebuf which is a countable release - if it does not get written + // downstream it needs to be released here + if (frame != null) frame.tryRelease(); + throw ex; } // the flush is called after the commit has potentially occurred. in this way, if a commit was
