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

fix: check closed status of socket during callbacks #257

Merged

Conversation

achingbrain
Copy link
Contributor

Sometimes calling getSendQueueCount can cause an internal crash in the dgram module.

I think it may be because packets are being sent during server shutdown - guarding on the this.closed property similar to the sendPacket method avoids the crash.

TypeError: Cannot read properties of null (reading 'getSendQueueCount')
    at Socket.getSendQueueCount (node:dgram:995:36)
    at Http3WebTransportClientSocket.packetSendCB (file:///Users/alex/Documents/Workspaces/libp2p/js-libp2p-webtransport-listener/node_modules/@fails-components/webtransport-transport-http3-quiche/lib/socket.js:118:24)
    at processTicksAndRejections (node:internal/process/task_queues:82:21)

Sometimes calling `getSendQueueCount` can cause an internal crash,
I think it may be because packets are being sent during server
shutdown - guarding on the `this.closed` property similar to the
`sendPacket` method avoids the crash.
@martenrichter
Copy link
Member

Sure, I have also seen this. But it was during another problem, so I thought it was a side effect.

@martenrichter martenrichter merged commit 4cda654 into fails-components:master Mar 4, 2024
4 checks passed
@martenrichter
Copy link
Member

Sorry I did not think this through, I reverted the PR.
This may cause promises to be not resolved.
The protection must to be only in the check, if writes are blocked.

@martenrichter
Copy link
Member

Btw, I think this is a bug, which also should be filed with node.js.

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