Skip to content
This repository was archived by the owner on Mar 3, 2026. It is now read-only.

Handling exceptions from user-supplied lambdas in GraphRequestAction #18

Open
dalaro wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Handling exceptions from user-supplied lambdas in GraphRequestAction #18

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d7ddb07
Handle buildFromSession exceptions in GraphRequestAction
dalaro Nov 6, 2019
1f7a9c9
Test GraphRequestAction after buildFromSession failure
dalaro Nov 6, 2019
d8871c8
Move GraphStatement lambda exception handling
dalaro Nov 7, 2019
3929115
Test + comment fixup for GraphStatement lambda handling
dalaro Nov 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion src/main/scala/com/datastax/gatling/plugin/request/GraphRequestAction.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,14 @@ import java.util.concurrent.ExecutorService
import java.util.concurrent.TimeUnit.MICROSECONDS

import akka.actor.ActorSystem
import com.datastax.driver.dse.graph.GraphStatement
import com.datastax.gatling.plugin.DseProtocol
import com.datastax.gatling.plugin.metrics.MetricsLogger
import com.datastax.gatling.plugin.model.DseGraphAttributes
import com.datastax.gatling.plugin.response.GraphResponseHandler
import com.datastax.gatling.plugin.utils._
import io.gatling.commons.stats.KO
import io.gatling.commons.validation.Validation
import io.gatling.core.action.{Action, ExitableAction}
import io.gatling.core.session.Session
import io.gatling.core.stats.StatsEngine
Expand Down Expand Up @@ -69,7 +71,18 @@ class GraphRequestAction(val name: String,
ThroughputVerifier.checkForGatlingOverloading(session, gatlingTimingSource)
GatlingResponseTime.startedByGatling(session, gatlingTimingSource)
}
val stmt = dseAttributes.statement.buildFromSession(session)

// Attempt to generate a graph statement from our parameters, propagating any uncaught exceptions after
// continuing the chain with a failed session
val stmt: Validation[GraphStatement] = try {
dseAttributes.statement.buildFromSession(session)

@pingtimeout pingtimeout Nov 7, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the comment lines are not accurate anymore here, could you restore the original line?

@dalaro dalaro Nov 7, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, I'll delete that comment

} catch {
case e: Throwable => {
logger.error("Failed to generate GraphStatement", e)
next ! session.markAsFailed
throw e
}
}

@pingtimeout pingtimeout Nov 6, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You wrapped the call dseAttributes.statement.buildFromSession(session) with a try catch. But according to the type system, this call should return a Validation[GraphStatement]. A Validation is a Gatling structure that represents either a Success or Failure (http://gatling.io/docs/current/session/validation/). So really, this case of an uncaught exception should have been handled before we return to GraphRequestAction.

I think that the fix should be located in DseGraphStatements.scala:54 instead of here. Proposition: replace L54 in DseGraphStatements by safely()(lambda(gatlingSession).success). This will use a built-in Gatling function that does exactly what you want, while also satisfying the type system.

@dalaro dalaro Nov 7, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the insight.

Gatling's safely is a lot more concise and idiomatic than try-catch. There's one thing that makes me uneasy about it though -- I don't see a way to log the stacktrace of the exception caught by safely before it goes out of scope. For a user-provided lambda, there could be cases where the exception stacktrace is useful, potentially significantly more useful than the exception type/message (e.g. NPE).

I just pushed a new commit that moves exception handling down to DseGraphStatements.scala:54 as directed, but uses Try { ... } match { ... } instead of safely. It's now similar to some of its peer implementations inside DseGraphStatements.scala that also already used similar Try { ... } match { ... } constructs, and I can still see lambda traces in the logs.

What do you think?

Theoretically, pushing down exception handling like this could allow some other implementation of buildFromSession to induce a hang, if that implementation allows uncaught exceptions to propagate out to the caller; but it looks like some of the other implementations in DseGraphStatements already go out of their way to prevent that, and I'm assuming your comment means that's the general rule.

@pingtimeout pingtimeout Nov 7, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a way to log the stacktrace of the exception caught by safely before it goes out of scope.

That's a good reason to use a try/catch block, indeed. +1 for that approach

Theoretically, pushing down exception handling like this could allow some other implementation of buildFromSession to induce a hang

I agree with you. I also wondered whether to keep the extra try/catch in GraphRequestAction, just to be 100% future proof. But I think the way to go is to honor the type system and make it clear that errors are prevented.

@dalaro dalaro Nov 7, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you. I also wondered whether to keep the extra try/catch in GraphRequestAction, just to be 100% future proof. But I think the way to go is to honor the type system and make it clear that errors are prevented.

Sounds good, I'll stick to the pushdown approach and update the tests to match. Thank you for addressing this.


stmt.onFailure(err => {
val responseTime = responseTimeBuilder.build()
Expand Down
83 changes: 65 additions & 18 deletions src/test/scala/com/datastax/gatling/plugin/request/GraphRequestActionSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ import java.util.concurrent.{Executor, TimeUnit}

import akka.actor.{ActorSystem, Props}
import akka.testkit.TestKitBase
import ch.qos.logback.classic.{Level, Logger}
import ch.qos.logback.classic.spi.ILoggingEvent
import ch.qos.logback.core.read.ListAppender
import com.datastax.driver.core._
import com.datastax.driver.dse.DseSession
import com.datastax.driver.dse.graph.{GraphResultSet, RegularGraphStatement, SimpleGraphStatement}
Expand All @@ -14,12 +17,13 @@ import com.datastax.gatling.plugin.DseProtocol
import com.datastax.gatling.plugin.model.{DseGraphStatement, DseGraphAttributes}
import com.google.common.util.concurrent.{Futures, ListenableFuture}
import io.gatling.commons.validation.SuccessWrapper
import io.gatling.core.action.Exit
import io.gatling.core.action.{Action, ChainableAction, Exit}
import io.gatling.core.config.GatlingConfiguration
import io.gatling.core.session.Session
import io.gatling.core.stats.StatsEngine
import org.easymock.EasyMock
import org.easymock.EasyMock._
import org.slf4j.LoggerFactory

class GraphRequestActionSpec extends BaseSpec with TestKitBase {
implicit lazy val system = ActorSystem()
Expand All @@ -30,10 +34,11 @@ class GraphRequestActionSpec extends BaseSpec with TestKitBase {
val statsEngine: StatsEngine = mock[StatsEngine]
val gatlingSession = Session("scenario", 1)

def getTarget(dseAttributes: DseGraphAttributes): GraphRequestAction = {
def getTarget(dseAttributes: DseGraphAttributes,
next: Action = new Exit(system.actorOf(Props[DseRequestActor]), statsEngine)): GraphRequestAction = {
new GraphRequestAction(
"sample-dse-request",
new Exit(system.actorOf(Props[DseRequestActor]), statsEngine),
next,
system,
statsEngine,
DseProtocol(dseSession),
Expand Down Expand Up @@ -62,24 +67,27 @@ class GraphRequestActionSpec extends BaseSpec with TestKitBase {
override protected def afterAll(): Unit = {
shutdown(system)
}


private def getTestGraphAttributes(): DseGraphAttributes =
DseGraphAttributes("test", dseGraphStatement,
cl = Some(ConsistencyLevel.ANY),
userOrRole = Some("test_user"),
readTimeout = Some(12),
defaultTimestamp = Some(1498167845000L),
idempotent = Some(true),
readCL = Some(ConsistencyLevel.LOCAL_QUORUM),
writeCL = Some(ConsistencyLevel.LOCAL_QUORUM),
graphName = Some("MyGraph"),
graphLanguage = Some("english"),
graphSource = Some("mysource"),
graphInternalOptions = Some(Seq(("get", "this"))),
graphTransformResults = None
)

describe("Graph") {
val statementCapture = EasyMock.newCapture[RegularGraphStatement]
it("should enable all the Graph Attributes in DseAttributes") {
val graphAttributes = DseGraphAttributes("test", dseGraphStatement,
cl = Some(ConsistencyLevel.ANY),
userOrRole = Some("test_user"),
readTimeout = Some(12),
defaultTimestamp = Some(1498167845000L),
idempotent = Some(true),
readCL = Some(ConsistencyLevel.LOCAL_QUORUM),
writeCL = Some(ConsistencyLevel.LOCAL_QUORUM),
graphName = Some("MyGraph"),
graphLanguage = Some("english"),
graphSource = Some("mysource"),
graphInternalOptions = Some(Seq(("get", "this"))),
graphTransformResults = None
)
val graphAttributes = getTestGraphAttributes()

expecting {
dseGraphStatement.buildFromSession(gatlingSession).andReturn(new SimpleGraphStatement("g.V()").success)
Expand All @@ -105,6 +113,45 @@ class GraphRequestActionSpec extends BaseSpec with TestKitBase {
capturedStatement.getGraphInternalOption("get") shouldBe "this"
}

it("should continue the ChainableAction sequence if the graph statement builder throws an exception") {
val initialGatlingSession = mock[Session]
val failedGatlingSession = mock[Session]
val nextChainedAction = mock[ChainableAction]
val userExceptionMessage = "this is safe to ignore " +
"(it simulates a broken user-supplied lambda for generating a GraphStatement)"
val graphAttributes = getTestGraphAttributes()

val classLogger = LoggerFactory.getLogger(classOf[GraphRequestAction]).asInstanceOf[Logger]
val listAppender: ListAppender[ILoggingEvent] = new ListAppender[ILoggingEvent]
listAppender.start()
classLogger.addAppender(listAppender)

expecting {
dseGraphStatement.buildFromSession(initialGatlingSession).andThrow(
new RuntimeException(userExceptionMessage)
)
initialGatlingSession.startDate.andReturn(42L) // This number has no significance here, we could stub anything
initialGatlingSession.markAsFailed.andReturn(failedGatlingSession)
nextChainedAction ! failedGatlingSession
}

whenExecuting(dseGraphStatement, initialGatlingSession, failedGatlingSession, nextChainedAction) {
intercept[RuntimeException] {
getTarget(graphAttributes, nextChainedAction).sendQuery(initialGatlingSession)
}
}

assert(listAppender.list.size() == 1)

val logEntry = listAppender.list.get(0)

assert(logEntry.getLevel == Level.ERROR)
assert(logEntry.getFormattedMessage.contains("Failed to generate GraphStatement"))
assert(logEntry.getThrowableProxy != null)
assert(logEntry.getThrowableProxy.getMessage().equals(userExceptionMessage))
assert(logEntry.getThrowableProxy.getClassName().equals(classOf[RuntimeException].getCanonicalName()))
}

it("should override the graph name if system") {
val graphAttributes = DseGraphAttributes(
"test",
Expand Down