Do not close connection on GrapQL query error #18253
Conversation
glyh
force-pushed
the
lyh/do-not-close-connection-on-graphql-query-error
branch
2 times, most recently
from
e195ef0 to
1aac662
Compare
| | Ok doc -> | ||
| Schema.execute schema ctx ?variables ?operation_name doc | ||
| let open Async in | ||
| Monitor.try_with (fun () -> |
There was a problem hiding this comment.
We have to use a Monitor.try_with because try .. with doesn't work with Async.
| struct | ||
| module Ws = Websocket.Connection.Make (Io) | ||
| module Websocket_transport = Websocket_handler.Make (Schema.Io) (Ws) | ||
| module Make = struct |
There was a problem hiding this comment.
This could be further refactored. But I think it'll be better for us to take ownership of the upstream library ocaml-graphql-server and patch there:
- This file is copy-pasted and modified by us from that lib;
- Nobody is maintaining that library at all.
I'd definitely save this for a follow up PR if ever.
| module Make | ||
| (Schema : Graphql_intf.Schema) | ||
| (Io : Cohttp.S.IO with type 'a t = 'a Schema.Io.t) | ||
| (Body : HttpBody with type +'a io := 'a Schema.Io.t) = |
There was a problem hiding this comment.
Because we used Monitor.protect within this functor, the abstraction doesn't work -- This implementation would only work for Async. Not any other libs like, say, Lwt.
…own when resovling GQL schema
glyh
force-pushed
the
lyh/do-not-close-connection-on-graphql-query-error
branch
from
1aac662 to
6a10221
Compare
|
!ci-docker-me codename=noble arch=amd64 profile=devnet |
|
!ci-docker-me codename=noble arch=amd64 profile=mainnet |
glyh
force-pushed
the
lyh/do-not-close-connection-on-graphql-query-error
branch
from
f6dda49 to
32e5263
Compare
glyh
force-pushed
the
lyh/do-not-close-connection-on-graphql-query-error
branch
from
32e5263 to
fc26c61
Compare
|
!ci-build-me |
glyh
force-pushed
the
lyh/do-not-close-connection-on-graphql-query-error
branch
from
fc26c61 to
b121d9e
Compare
|
!ci-build-me |
| fi | ||
|
|
||
| # Check bestTip | ||
| BEST_TIP_STATE_HASH=$(curl -f --show-error 'http://localhost:3085/graphql' \ |
There was a problem hiding this comment.
Looks like everything before --data-raw is duplicated between the request. Please extract that part to some common string maybe?
| done | ||
|
|
||
| # Check that the daemon has connected to peers and is still up after 2 mins | ||
| if [[ "$node_status" != "SYNCED" ]]; then |
There was a problem hiding this comment.
we're checking for node to synced instead of peers now
|
!ci-build-me |
|
Should be ready for another round of review. |
This is inspired by #18247. It does NOT close it, because there seems to be some other issues stemming from Nori's observation.
Admitted this is a very ugly fix, in that we just wrap the error with 500 and error message as the body. However, to proper handle errors, it requires a lot of refactor, which I don't think we could afford now.
Status: waiting for review.