Comments (10)
Let me elaborate this a bit:
The problem is that lacinia's use of future
(per link above) prevents the application from leveraging Thread/setDefaultUncaughtExceptionHandler
(see Stuart Sierra's blog post https://stuartsierra.com/2015/05/27/clojure-uncaught-exceptions).
Any exceptions bubbling from user registered callback via (on-deliver! result-promise callback)
(which could be arbitrary code, like sending the response over HTTP), will get caught by future
. The Future is then set to failed state, but given the object is not available outside of execute-query
you can't recover the exception. It's a silent failure, no stack trace, no nothing.
If instead the exception was allowed to bubble all the way to the top of the stack you could use Thread/setDefaultUncaughtExceptionHandler
to log it. To achieve that, lacinia needs to avoid future
and schedule tasks using Executor/execute
instead. As it happens, lacinia does exactly that if a *callback-executor*
is provided.
So, the fix could be as easy as defaulting *callback-executor*
to clojure.lang.Agent/soloExecutor
(the executor used by future
). I'm happy to provide a PR if maintainers are happy with this approach.
PS. Admittedly Thread/setDefaultUncaughtExceptionHandler
is a band-aid. It could be argued that callbacks should just not be allowed to throw. In async code, with inverted call stack, there's just nothing useful an application can really do with exceptions that logically happen way outside of its scope. But in real world mistakes happen, and visibility is key for debugging.
from lacinia.
I'll have to refresh my memory on future behavior; essentially I'd prefer to capture the exception in the future's thread, and re-throw the exception when derefing the future; that would more likely be on the Pedestal stack which is a better place to throw exceptions (the interceptor stack may include something to report the exception and return a 500 status response).
from lacinia.
essentially I'd prefer to capture the exception in the future's thread, and re-throw the exception when derefing the future
In this instance, future
is never returned to the caller, so there is no opportunity to deref
. The reference to that object is effectively lost. As used in the code linked above, future
is merely a convenient way to kick off a background task.
The argument is that the pattern of using future
for background tasks you're not awaiting the result value of is problematic. That's precisely because the exception will be captured in an unreachable Future
object, with no opportunity to get discovered.
from lacinia.
The f
in (future (f))
has a (try ...
) block in it that ensure that exceptions are caught and returned, and the higher levels check to see if the nominal response map is actually an exception.
The problem with allowing exceptions to escape to the thread's uncaught exception handler is that other code, that is waiting for that asynchronous block of code to execute, will wait forever unless they build there own explicit timeout. That means that, in lacinia-pedestal, you end up with a request thread that blocks, or (if async) a request that never gets serviced.
On the other hand, if an exception occurs before it gets to line 421, that will be executed in the caller's thread, where it can be caught normally. In a Pedestal request processing thread, that will be caught and passed through the interceptor stack, to a point where that 500 response can be sent to the client.
So I fail to see the real problem here, is this some theoretical objection, or is there a particular use case in your application that I'm unaware of?
from lacinia.
If I remember well the reasoning was that this line inside the catch block can throw something
, becauseresolve/deliver!
calls the :callback
fn of the state and that opens the door to .... literally anything?
Regarding the theoretical part, even when you fix this by wrapping the inside of catch with another try-catch and you do something super safe in the second level catch you force the reader of code to check if the calling of f
is safe of not. And that is a very practical concern. Not using future
without deref
makes the writers and readers life easier, as far as error handling goes, I think.
from lacinia.
I'm thinking that the right approach here would be to add a schema/compile
option for an executor used with resolver result (it would provide one by default). Then that can be expected to exist, and we can always submit to that executor and not use a future. That's a bit more elegant.
It would have been nice if resolve/resolve-promise
took the context as an argument, so there wouldn't be a need to bind *callback-executor*
.
from lacinia.
That sounds quite right 👍
from lacinia.
So, what do you think of #424 ?
from lacinia.
👍 I think it solves the issue.
from lacinia.
Fixed by #424.
from lacinia.
Related Issues (20)
- icu4j vulnerability HOT 1
- Scalar type which returns vector HOT 4
- ::lacinia/null returned in unions instead of null HOT 1
- Apollo Federation Compatibility HOT 1
- Includes variable-definitions in parsed query HOT 4
- Question: Is there a way to return JSON without a corresponding EDN definition? HOT 3
- Use of executor/selection in streamer functions HOT 1
- Document example of a raw JSON scalar
- cryptic errors when input is the wrong type
- Passing in variables wrapped in an array comes in as a double array HOT 5
- Support @defer and @stream directives
- Convert the keys of arguments and variables to kebab-case HOT 1
- Subscriptions may fail to stream data to the client
- Unbalanced braces in queries not causing parse-query to fail
- Tracing "validation" fields are always null
- Question: Query complexity and depth HOT 2
- Replace antlr4 deps with antlr4-runtime HOT 2
- Issue with merging fragments with list
- Adding support for deprecation on input fields
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from lacinia.