fix(browser): prevent error stream.push() after EOF

[sanduluca]

check if the stream is still open and writable before pushing data.

Also I was wondering if a Event Listener Cleanup should be made ? I dont see any removeEventListener to prevent any further callbacks from firing after closure:

    socket.removeEventListener('close', onClose);
    socket.removeEventListener('error', onError);
    socket.removeEventListener('message', onMessage);
    socket.removeEventListener('open', onOpen);

Fixes #1914

[sanduluca]

Hmm 🤔
nodejs/node#445

[robertsLando]

@sanduluca Thanks for your PR!

About the event listeners, if you are speaking about Websocket event listeners yes it could make sense to remove them on destroy anyway once the socket is destroyed it should be garbage collected and should not create any problem but 🤷🏼‍♂️

[robertsLando]

Hmm 🤔 nodejs/node#445

It's almost 10 years old now 😆 Anyway I wonder so if there is a better way to handle this? Does this change solve your issue?

[robertsLando]

cc @mcollina could you give a look at this please?

[robertsLando]

@mcollina ping

[KirillRodichevUtorg]

@sanduluca could you explain why did you stick to proxy._writableState.ended and not proxy._readableState.ended. from what I understand, proxy._writableState.ended indicates that the writable side is ended, but proxy.push() is an operation on the readable side

FYI @robertsLando

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 81.15%. Comparing base (d7553e3) to head (2c0a144).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/lib/connect/ws.ts	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1932      +/-   ##
==========================================
- Coverage   81.21%   81.15%   -0.06%     
==========================================
  Files          24       24              
  Lines        1469     1470       +1     
  Branches      349      349              
==========================================
  Hits         1193     1193              
- Misses        192      193       +1     
  Partials       84       84              

Flag	Coverage Δ
unittests	81.15% <0.00%> (-0.06%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[robertsLando]

@sanduluca could you explain why did you stick to proxy._writableState.ended and not proxy._readableState.ended. from what I understand, proxy._writableState.ended indicates that the writable side is ended, but proxy.push() is an operation on the readable side

FYI @robertsLando

Yes the push is an operation on readable side but I think checking for readable or writeable here could be the same, I agree it would be better to change the check and use readableState instead

[KirillRodichevUtorg]

@robertsLando I made some experiments that show that we better use readable here

import { Duplex } from 'stream';

class MyDuplex extends Duplex {
    constructor() {
        super();
        this.data = ['Hello', 'World'];
    }

    // Implement the _read method to supply data to the readable side
    _read() {
        console.log('Readable called');
        if (this.data.length > 0) {
            this.push(this.data.shift()); // Push data to the readable side
        } else {
            this.push(null); // End the readable side
        }
    }

    // Implement the _write method for writable side
    _write(chunk, encoding, callback) {
        console.log(`Writable received: ${chunk.toString()}`);
        callback();
    }
}

// Create an instance of MyDuplex
const duplex = new MyDuplex();

console.log('Before push(null):', duplex.readable); // true
console.log('Before push(null):', duplex.writable); // true

// Consume the readable data
duplex.on('data', (chunk) => {
    console.log(`Read: ${chunk.toString()}`);
});

// Listen to the 'end' event to detect when the readable side ends
duplex.on('end', () => {
    console.log('Stream ended');
    console.log('After push(null):', duplex.readable, duplex._readableState.ended); // false true
    console.log('After push(null):', duplex.writable, duplex._writableState.ended); // true false
});

[robertsLando]

@KirillRodichevUtorg Could you create a new PR with your fixed version of this PR? Seems @sanduluca is not available so that would be faster

[sanduluca]

I dont remeber why I did what I did, but thanks for pointing this out!

Thanks for your experiments. It shows that the proxy is not destroyed but already not readable.

I made the changes to the PR, just in case.

[robertsLando]

No clue why electron tests are failing but I'm quite sure that's not related to this PR. LGTM