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

catch connect error #246

Merged
merged 6 commits into from
Mar 11, 2023
Merged

catch connect error #246

merged 6 commits into from
Mar 11, 2023

Conversation

turuslan
Copy link
Contributor

@turuslan turuslan commented Feb 28, 2023

Deno does not allow unhandled promise rejection and terminates with error: Uncaught (in promise).

Added .catch to Deno.connect.

It calls config.onConnectionReset, like WebSocket (success is .onopen (then may .onerror) then .onclose, failure is .onerror then .onclose).

@turuslan turuslan requested a review from tomaka as a code owner February 28, 2023 19:10
@github-actions
Copy link

github-actions bot commented Feb 28, 2023

twiggy diff report

Difference in .wasm size before and after this pull request.


 Delta Bytes │ Item
─────────────┼──────────────────
          +0 ┊ Σ [0 Total Rows]

Copy link
Contributor

@tomaka tomaka left a comment

Choose a reason for hiding this comment

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

Thanks!

@@ -224,7 +228,7 @@ function connect(config: ConnectionConfig, forbidTcp: boolean, forbidWs: boolean
} else {
// TCP
connection.socket.destroyed = true;
connection.socket.inner.then((connec) => connec.close());
connection.socket.inner.then((connec) => connec?.close());
Copy link
Contributor

Choose a reason for hiding this comment

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

Can connec actually be null/undefined here? I feel like the only way that this could happen is if reset was called twice, which would be a smoldot bug

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I returned null when Deno.connect returns error, so added this null-check

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see, I had to re-read the documentation of Promise.catch to understand

I'm trying very hard to not lose track of what's happening, which can very easily happen with JavaScript.
Given that reset should never be called after onConnectionReset, this should never be null, so:

Suggested change
connection.socket.inner.then((connec) => connec?.close());
connection.socket.inner.then((connec) => connec!.close());

Comment on lines 164 to 167
}).catch((error) => {
socket.destroyed = true;
config.onConnectionReset(error.toString());
return null!;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you maybe move this as the second parameter of the then below?
This would guarantee that the first closure of the then never gets a null

Also:

Suggested change
}).catch((error) => {
socket.destroyed = true;
config.onConnectionReset(error.toString());
return null!;
}).catch((error) => {
socket.destroyed = true;
config.onConnectionReset(error.toString());
return null;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That first closure (and .send too) checks socket.destroyed, so doesn't use null.

Either null! is required, or change of type to TcpConnection { socket(inner: Promise<Deno.TcpConn | null>) }

Copy link
Contributor

Choose a reason for hiding this comment

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

or change of type to TcpConnection { socket(inner: Promise<Deno.TcpConn | null>) }

Well, yes, that option.
As far as I understand, null! lies to the type checker. It's an escape hatch for situations where TypeScript isn't good enough, and not something you're supposed to use normally.

@turuslan turuslan requested a review from tomaka March 8, 2023 08:54
@tomaka
Copy link
Contributor

tomaka commented Mar 11, 2023

I've pushed commits doing the change I've requested, plus a CHANGELOG entry

@tomaka
Copy link
Contributor

tomaka commented Mar 11, 2023

Thanks!

@tomaka tomaka enabled auto-merge March 11, 2023 10:16
@tomaka tomaka added this pull request to the merge queue Mar 11, 2023
Merged via the queue into smol-dot:main with commit ec76e50 Mar 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants