Stream read/write duplication

[mowchan]

Hello, I'm trying to open an interactive shell session and I'm running into an issue where I am seeing my input duplicated when it is piped back to process.stdout.

The code:

client.shell((err, stream) => {
    if (err) {
        console.error(err);
        process.exit(1);
    }
    // get each keystroke, not only new lines
    process.stdin.setRawMode(true);
    process.stdin.setEncoding('utf8');
    process.stdin.resume();
    // write keystrokes to the stream as they come in
    process.stdin.on('data', (key) => {
        if (key == '\u0003') {
            // exit on ctrl+c
            process.exit();
        }
        stream.write(key);
    });
    stream.pipe(process.stdout);
    // show uptime / let the user know the session is ready
    stream.write('uptime\n');
});

And the output I'm seeing:

uptime
uptime
 22:38:11 up  4:42,  load average: 1.86, 1.97, 1.99
~ # ff
-sh: f: not found
~ # ffoooo
-sh: foo: not found
~ # bbaarr
-sh: bar: not found

You can see that the text I typed in is duplicated (e.g. typed "foo", displayed as "ffoooo"), but the input was properly written to the stream ("-sh foo: not found").

Is there something I'm doing wrong or otherwise should be doing differently?

[shinhwa520]

I faced the same issue.
It's only do once "stream.write", but received twice "stream.on.data".

[shinhwa520]

Is there any solution to this problem?
Thank you.

[mkozjak]

Can you please try providing a PR to fix this issue? I cannot reproduce it on my end.

[jerrolb]

In the Stream class in index.js, I commented out this.push(data), and it fixed the problem. I used npm patch-package as a temporary solution. I'd have to dig a little bit to see what the PR should look like, but this might help shed some light.

this.source.write(data, encoding, () => {
// this.push(data)
cb()
})
}

[shinhwa520]

In the Stream class in index.js, I commented out this.push(data), and it fixed the problem. I used npm patch-package as a temporary solution. I'd have to dig a little bit to see what the PR should look like, but this might help shed some light.

this.source.write(data, encoding, () => {
// this.push(data)
cb()
})
}

I followed the method you provided, and it fixed the problem. 👍
However, does commenting out this line cause any side effects?
Thank you.

[jerrolb]

I'm not aware of any side effects, it seems to work as expected. Although, I haven't gotten around to looking at this further, so I can't guarantee that there are none.

[shinhwa520]

Got it. Thank you.

[shinhwa520]

@mkozjak
May I ask the reason why does it needs to do "push" on "source.write" listener?
Does it have a special purpose?
Will it cause any side effects if I delete this line?
Thank you.

[mkozjak]

What line and file is this?

…

On 11.03.2021., at 04:47, shinhwa520 ***@***.***> wrote:  @mkozjak May I ask the reason why does it needs to do "push" on "source.write" listener? Does it have a special purpose? Will it cause any side effects if I delete this line? Thank you. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[shinhwa520]

What line and file is this?
…
On 11.03.2021., at 04:47, shinhwa520 @.***> wrote:  @mkozjak May I ask the reason why does it needs to do "push" on "source.write" listener? Does it have a special purpose? Will it cause any side effects if I delete this line? Thank you. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

Sorry for not clarifying in my earlier comment.

The code is at line 445 in "lib/index.js" file.

class Stream extends Duplex {
  constructor(source, options) {
    super(options)
    this.source = source

    this.source.on('data', (data) => this.push(data))
  }

  _write(data, encoding, cb) {
    if (!this.source.writable) {
      cb(new Error('socket not writable'))
    }

    this.source.write(data, encoding, () => {
      this.push(data)   <<<<<<<<<<< this line
      cb()
    })
  }

  _read() {}
}

Thank you.

[mkozjak]

If you’re not using .shell function it should be safe. Run the tests.

[shinhwa520]

If you’re not using .shell function it should be safe. Run the tests.

I'm using the .shell function, but I tested the code that deletes the line and it seems to work fine.
But I am not sure if my test case covers all...

My source code is as below:

const Telnet = require('telnet-client');
...
let conn = new Telnet();
...

conn.shell((err, stream) => {
    socket.on('disconnecting', function socketOnDisconnecting(reason) {
        ...
    })
    socket.on('disconnect', function socketOnDisconnect(reason) {
        ...
    })
    socket.on('error', function socketOnError(err) {
        ...
    })

    stream.on('data', function streamOnData(data) {
        if (data.indexOf('Connection closed') !== -1 || data.indexOf('[ERROR]') !== -1) {
            socket.disconnect();
        } else {
            socket.emit('dataFromServer', data.toString());
        }
    })

    stream.on('close', function streamOnClose(code, signal) {
        socket.disconnect();
    })

    socket.on('dataFromWeb', function socketOnData(data) {
        stream.write(data);
    })
})

[mkozjak]

@shinhwa520 based on past comments, I feel it's okay to remove that line. Please provide a PR removing the arrow function altogether and instead of it, just provide cb (a third argument from _write(). Thanks!

[shinhwa520]

@shinhwa520 based on past comments, I feel it's okay to remove that line. Please provide a PR removing the arrow function altogether and instead of it, just provide cb (a third argument from _write(). Thanks!

Okay, I have already created a PR for this.
Please verify that I did not misunderstand what you mean.
Thank you.

[mkozjak]

Tackling this with #187.

@shinhwa520 Should we take care of the test 007_streams?

AssertionError:
'23:14  up 1 day, 21:50, 6 users, load averages: 1.41 1.43 1.41\r\n' +
  '/ # /dev/disk1     112Gi   87Gi   25Gi    78% 1913034 4293054245    0%   /\r\n' +
  '/ # '
==
'uptime\n' +
  '23:14  up 1 day, 21:50, 6 users, load averages: 1.41 1.43 1.41\r\n' +
  '/ # df\n' +
  '/dev/disk1     112Gi   87Gi   25Gi    78% 1913034 4293054245    0%   /\r\n' +
  '/ # '
    at Object.equals (/opt/dev/github.com/user/node-telnet-client/node_modules/nodeunit/lib/types.js:83:39)
    at /opt/dev/github.com/user/node-telnet-client/test/007_streams.js:65:18
    at tryCatcher (/opt/dev/github.com/user/node-telnet-client/node_modules/bluebird/js/release/util.js:16:23)
    at Promise._settlePromiseFromHandler (/opt/dev/github.com/user/node-telnet-client/node_modules/bluebird/js/release/promise.js:512:31)
    at Promise._settlePromise (/opt/dev/github.com/user/node-telnet-client/node_modules/bluebird/js/release/promise.js:569:18)
    at Promise._settlePromiseCtx (/opt/dev/github.com/user/node-telnet-client/node_modules/bluebird/js/release/promise.js:606:10)
    at _drainQueueStep (/opt/dev/github.com/user/node-telnet-client/node_modules/bluebird/js/release/async.js:142:12)
    at _drainQueue (/opt/dev/github.com/user/node-telnet-client/node_modules/bluebird/js/release/async.js:131:9)
    at Async._drainQueues (/opt/dev/github.com/user/node-telnet-client/node_modules/bluebird/js/release/async.js:147:5)
    at Immediate.Async.drainQueues (/opt/dev/github.com/user/node-telnet-client/node_modules/bluebird/js/release/async.js:17:14)
    at processImmediate (node:internal/timers:464:21)
    at process.topLevelDomainCallback (node:domain:147:15)
    at process.callbackTrampoline (node:internal/async_hooks:130:14)

[mkozjak]

@shinhwa520 Is this looking good?

#188

[mkozjak]

@shinhwa520 ?