Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better handling for user-defined error conditions in debug mode #116

Merged
merged 6 commits into from
Sep 6, 2019

Conversation

rpkyle
Copy link
Contributor

@rpkyle rpkyle commented Aug 13, 2019

As noted in #115, getStackTrace behaves a bit unexpectedly when a user defined error is encountered within a callback while in debug mode, e.g.

    if (city == "Winnipeg")
      stop(simpleError("Winnipeg?"))

ought to produce the following console text while sending a traceback up to the dev tools UI:

 ### DashR Traceback (most recent/innermost call last) ###
    1: getStackTrace
    2: do.call
    3: <anonymous> function(city, tempscale)
    4: stop
in debug mode, catching error as warning ...
warning: Execution error in stop: Winnipeg? from NULL

However, the traceback was not provided previously in such cases, and a JS error could be thrown in the browser instead, halting app execution.

This PR proposes to address the problem by introducing conditional logic to identify calls of class simpleError, and to deparse the calls so that they may be treated as strings when constructing the stack_message object.

The problem is that stop is a symbol in R, and attempting to naively treat it as a string may throw the following error:

Error in sprintf(template, header, throwing_call, error_message, formattedStack) : 
  invalid type of argument[2]: 'symbol'

which can prevent the traceback from being returned. The key additions are

dashR/R/utils.R

Lines 698 to 707 in b170cfa

if (!is.null(e$call[[1]]))
errorCall <- e$call[[1]]
else {
# attempt to capture the error or warning if thrown by
# simpleError or simpleWarning (which may arise for user-defined errors)
#
# the first matching call in the reversed stack will always be
# getStackTrace, so we select the second match instead
errorCall <- reverseStack[grepl(x=reverseStack, "simpleError|simpleWarning")][[2]]
}

dashR/R/utils.R

Lines 738 to 747 in b170cfa

if (is.function(currentCall[[1]])) {
identical(deparse(errorCall), deparse(currentCall[[1]]))
} else if (currentCall[[1]] == "stop") {
# handle case where function developer deliberately invokes a stop
# condition and halts function execution
identical(deparse(errorCall), deparse(currentCall))
}
else {
FALSE
}

dashR/R/utils.R

Lines 762 to 770 in b170cfa

# use deparse in case the call throwing the error is a symbol,
# since this cannot be "printed" without deparsing the call
warning(call. = FALSE, immediate. = TRUE, sprintf("Execution error in %s: %s",
deparse(functionsAsList[[length(functionsAsList)]]),
conditionMessage(e)))
stack_message <- stackTraceToHTML(functionsAsList,
deparse(functionsAsList[[length(functionsAsList)]]),
conditionMessage(e))

Closes #115.

@rpkyle rpkyle self-assigned this Aug 13, 2019
@rpkyle rpkyle added the bug label Aug 13, 2019
@rpkyle rpkyle changed the base branch from master to 112-prune-errors August 13, 2019 15:26
@rpkyle
Copy link
Contributor Author

rpkyle commented Aug 23, 2019

@alexcjohnson Any comments on this one?

@rpkyle
Copy link
Contributor Author

rpkyle commented Aug 23, 2019

As discussed this morning, I'll implement a basic test which incorporates a stop error to demonstrate what this resolves.

@rpkyle rpkyle changed the base branch from 112-prune-errors to dev September 3, 2019 19:07
@rpkyle
Copy link
Contributor Author

rpkyle commented Sep 6, 2019

As discussed this morning, I'll implement a basic test which incorporates a stop error to demonstrate what this resolves.

fixed in 1029ec4

@alexcjohnson

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

💃

@rpkyle rpkyle merged commit 29a3042 into dev Sep 6, 2019
@rpkyle rpkyle deleted the 115-handle-stop-errors branch September 6, 2019 20:36
@rpkyle rpkyle mentioned this pull request Jan 3, 2020
rpkyle added a commit that referenced this pull request Jan 4, 2020
* Provide support for no_update in Dash for R (#111)

* Use dev_tools_prune_errors instead of pruned_errors (#113)

* Better handling for user-defined error conditions in debug mode (#116)

* Provide support for multiple outputs (#119)

* Provide support for hot reloading in Dash for R (#127)

* Implement support for clientside callbacks in Dash for R (#130)

* Add line number context to stack traces when srcrefs are available (#133)

* Update dash-renderer to 1.2.2 and fix dev tools UI display of stack traces (#137)

* Support for meta tags in Dash for R (#142)

* Fixes for hot reloading interval handling and refreshing apps within viewer pane (#148)

* Support for asynchronous loading/compression in Dash for R (#157)

* Support returning asset URLs via public method within Dash class (#160)

* Minor fix for get_asset_url + docs, add url_base_pathname (#161)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants