search

surrealdb / surrealdb.js

SurrealDB SDK for JavaScript
https://surrealdb.com
Apache License 2.0
272 stars 45 forks source link

fix: reject send promise upon catching inner error #220

Closed oliver-oloughlin closed 3 months ago

oliver-oloughlin commented 3 months ago

What is the motivation?

Currently, errors that occur within SurrealSocket.send() are not surfaced properly, making them uncatchable. This leads to issues such as crashing a user's program if an error occurs, without letting them re-establish the database connection or continue execution gracefully.

What does this change do?

This PR catches any inner errors, upon which the outer send() promise is rejected with the subsequent error. This allows for the error to be surfaced and caught.

What is your testing strategy?

I tested this change by running an infinite loop that would try to insert data into a running in-memory SurrealDB instance. The insert statement was surrounded by a try/catch block which would break the loop upon catching any errors and print a console.log message. While the loop was running, I would stop the SurrealDB instance and observe the results in the terminal. Before the change, the program would simply crash with the uncaught error. After the change, the loop would break gracefully and the console.log would be displayed, indicating the error had been caught and handled as expected. I also tried having the database re-establish the connection after a 5 second timeout instead of breaking the loop, where I would restart the database instance before the timeout would end. When doing this, the loop started running again, successfully inserting data.

Is this related to any issues?

These changes are related to #203. Ths PR does nothing directly to re-establish the database connection upon losing it, but it does surface otherwise uncatchable errors, allowing for the user to manually re-establish the connection.

Have you read the Contributing Guidelines?

oliver-oloughlin commented 3 months ago

I might add that I think a better solution would be to restructure the current code somehow such that there isn't an unawaited promise when calling the SurrealSocket.send() method from within WebSocketStrategy.send(). However, this would certainly require more work and be a much more intrusive change. I believe the current proposed change does effectively address the issue with minimal changes to the code.

oliver-oloughlin commented 3 months ago

I went ahead and created an alternative change branch which takes a different approach and doesn't leave the inner promise unawaited, here: https://github.com/oliver-oloughlin/surrealdb.js/tree/bugfix-203-ensure-ws-errors-are-surfaced-2

Maybe worth considering?

mudlee commented 3 months ago

I don't know how to upvote, but I'm upvoting implicitly here.

oliver-oloughlin commented 3 months ago

@kearfy Hi, would you be able to give a review of this PR please? :)

kearfy commented 3 months ago

Hey everyone! Apologies for the delay here. We've been working on a refresh of the driver in a jsv2 branch, however I'll merge this first and get it out in a patch release. Thanks for this @oliver-oloughlin, great find!

oliver-oloughlin commented 3 months ago

Seems the workflows are failing, but not sure if its related to this PR or not?

The linting error is about the assert keyword being deprecated, which should be replaced with with. The tests fail when I run them locally on the main branch as well, without any changes applied. There is also a type error in compile.ts when using deno check regarding the compiler options, where lib: ["dom"] should be lib: ["DOM"].

Seems like there are some issues that maybe need fixing before this can be merged? Or alternatively, if you are creating a refreshed driver anyway, just utilize what we have learned here when re-implementing it?