Skip to content

flaky test.parallel/test-diagnostics-channel-net #44143

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
kvakil opened this issue Aug 5, 2022 · 6 comments · Fixed by #44144 or #44154
Closed

flaky test.parallel/test-diagnostics-channel-net #44143

kvakil opened this issue Aug 5, 2022 · 6 comments · Fixed by #44144 or #44154
Labels
flaky-test Issues and PRs related to the tests with unstable failures on the CI. net Issues and PRs related to the net subsystem.

Comments

@kvakil
Copy link
Contributor

kvakil commented Aug 5, 2022

Test

test-diagnostics-channel-net

Platform

Linux x64

Console output

00:32:11 not ok 653 parallel/test-diagnostics-channel-net
00:32:11   ---
00:32:11   duration_ms: 0.269
00:32:11   severity: fail
00:32:11   exitcode: 1
00:32:11   stack: |-
00:32:11     Mismatched <anonymous> function calls. Expected exactly 1, actual 0.
00:32:11   ...

Build links

Additional information

  1. Potentially only happens in worker test suite?
  2. Could not reproduce locally with:
for i in {1..10000}; do
  ./node --abort-on-uncaught-exception \
     ./tools/run-worker.js test/parallel/test-diagnostics-channel-net.js \
     || echo "fail $i" &
done |& grep "fail "
  1. Not clear to me which callback is not getting invoked.
@kvakil kvakil added the flaky-test Issues and PRs related to the tests with unstable failures on the CI. label Aug 5, 2022
@kvakil
Copy link
Contributor Author

kvakil commented Aug 5, 2022

@theanarkh any thoughts? It's weird also that the stack trace does not include any indication what's not being called. :\

edit: never mind, I think I found it: #44144

@kvakil kvakil added the net Issues and PRs related to the net subsystem. label Aug 5, 2022
kvakil added a commit to kvakil/node that referenced this issue Aug 5, 2022
This test uses the deprecated methods in `diagnostic_channel` (see the
reference), which are deprecated because the channels can be GC'd while
in use, leading to lost messages.

Change to use the non-deprecated APIs, which should work. I wasn't able
to reproduce this locally; I assume it's memory dependent.

Refs: nodejs#42714
Fixes: nodejs#44143
@theanarkh
Copy link
Contributor

theanarkh commented Aug 5, 2022

Thanks. I have no ideas now, i will take a look later. But i think it is not related to gc because this case will keep the channel object alive. It is different from #42170. cc @Qard.
.

@theanarkh
Copy link
Contributor

@Qard Hi, what do you think about this case. Is it the wrong way to use diagnostics_channel?

@Qard
Copy link
Member

Qard commented Aug 5, 2022

Already approved the fix in #44144.

@theanarkh
Copy link
Contributor

But there's a lot of usage in the code, and if there's a problem, it all needs to be fixed ?
image

@theanarkh
Copy link
Contributor

theanarkh commented Aug 6, 2022

Running the code as follows will trigger this error always(./node --expose-gc test/parallel/test-diagnostics-channel-net.js). The callback of onGC will be triggered before timeout.

'use strict';
const common = require('../common');
const assert = require('assert');
const net = require('net');
const dc = require('diagnostics_channel');
const onGC = require('../common/ongc');
const { writeFileSync } = require('fs');

const netClientSocketChannel = dc.channel('net.client.socket');
const netServerSocketChannel = dc.channel('net.server.socket');

onGC(netClientSocketChannel, { 
  ongc: () => {
    writeFileSync(1, "gc netClientSocketChannel");
  }
});

onGC(netServerSocketChannel, {
  ongc: () => {
    writeFileSync(1, "gc netServerSocketChannel");
  }
});

const isNetSocket = (socket) => socket instanceof net.Socket;

netServerSocketChannel.subscribe(common.mustCall(({ socket }) => {
  assert.strictEqual(isNetSocket(socket), true);
}));

netServerSocketChannel.subscribe(common.mustCall(({ socket }) => {
  assert.strictEqual(isNetSocket(socket), true);
}));
gc();
setTimeout(() => {
  console.log('timeout emit');
  const server = net.createServer(common.mustCall((socket) => {
    socket.destroy();
    server.close();
  }));
  
  server.listen(() => {
    const { port } = server.address();
    net.connect(port);
  });
}, 10000);

it is related to gc indeed. I will send a PR to fix this.Thanks !

nodejs-github-bot pushed a commit that referenced this issue Aug 7, 2022
This test uses the deprecated methods in `diagnostic_channel` (see the
reference), which are deprecated because the channels can be GC'd while
in use, leading to lost messages.

Change to use the non-deprecated APIs, which should work. I wasn't able
to reproduce this locally; I assume it's memory dependent.

Refs: #42714
Fixes: #44143
PR-URL: #44144
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
nodejs-github-bot pushed a commit that referenced this issue Aug 8, 2022
PR-URL: #44154
Fixes: #44143
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
danielleadams pushed a commit that referenced this issue Aug 16, 2022
PR-URL: #44154
Fixes: #44143
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
ruyadorno pushed a commit that referenced this issue Aug 23, 2022
PR-URL: #44154
Fixes: #44143
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
ruyadorno pushed a commit that referenced this issue Aug 23, 2022
This test uses the deprecated methods in `diagnostic_channel` (see the
reference), which are deprecated because the channels can be GC'd while
in use, leading to lost messages.

Change to use the non-deprecated APIs, which should work. I wasn't able
to reproduce this locally; I assume it's memory dependent.

Refs: #42714
Fixes: #44143
PR-URL: #44144
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
targos pushed a commit that referenced this issue Sep 5, 2022
PR-URL: #44154
Fixes: #44143
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
targos pushed a commit that referenced this issue Sep 5, 2022
This test uses the deprecated methods in `diagnostic_channel` (see the
reference), which are deprecated because the channels can be GC'd while
in use, leading to lost messages.

Change to use the non-deprecated APIs, which should work. I wasn't able
to reproduce this locally; I assume it's memory dependent.

Refs: #42714
Fixes: #44143
PR-URL: #44144
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Fyko pushed a commit to Fyko/node that referenced this issue Sep 15, 2022
This test uses the deprecated methods in `diagnostic_channel` (see the
reference), which are deprecated because the channels can be GC'd while
in use, leading to lost messages.

Change to use the non-deprecated APIs, which should work. I wasn't able
to reproduce this locally; I assume it's memory dependent.

Refs: nodejs#42714
Fixes: nodejs#44143
PR-URL: nodejs#44144
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Fyko pushed a commit to Fyko/node that referenced this issue Sep 15, 2022
PR-URL: nodejs#44154
Fixes: nodejs#44143
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
juanarbol pushed a commit that referenced this issue Oct 10, 2022
PR-URL: #44154
Fixes: #44143
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
juanarbol pushed a commit that referenced this issue Oct 10, 2022
This test uses the deprecated methods in `diagnostic_channel` (see the
reference), which are deprecated because the channels can be GC'd while
in use, leading to lost messages.

Change to use the non-deprecated APIs, which should work. I wasn't able
to reproduce this locally; I assume it's memory dependent.

Refs: #42714
Fixes: #44143
PR-URL: #44144
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
juanarbol pushed a commit that referenced this issue Oct 11, 2022
PR-URL: #44154
Fixes: #44143
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
juanarbol pushed a commit that referenced this issue Oct 11, 2022
This test uses the deprecated methods in `diagnostic_channel` (see the
reference), which are deprecated because the channels can be GC'd while
in use, leading to lost messages.

Change to use the non-deprecated APIs, which should work. I wasn't able
to reproduce this locally; I assume it's memory dependent.

Refs: #42714
Fixes: #44143
PR-URL: #44144
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
guangwong pushed a commit to noslate-project/node that referenced this issue Jan 3, 2023
PR-URL: nodejs/node#44154
Fixes: nodejs/node#44143
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
guangwong pushed a commit to noslate-project/node that referenced this issue Jan 3, 2023
This test uses the deprecated methods in `diagnostic_channel` (see the
reference), which are deprecated because the channels can be GC'd while
in use, leading to lost messages.

Change to use the non-deprecated APIs, which should work. I wasn't able
to reproduce this locally; I assume it's memory dependent.

Refs: nodejs/node#42714
Fixes: nodejs/node#44143
PR-URL: nodejs/node#44144
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
guangwong pushed a commit to noslate-project/node that referenced this issue Jan 3, 2023
PR-URL: nodejs/node#44154
Fixes: nodejs/node#44143
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
guangwong pushed a commit to noslate-project/node that referenced this issue Jan 3, 2023
This test uses the deprecated methods in `diagnostic_channel` (see the
reference), which are deprecated because the channels can be GC'd while
in use, leading to lost messages.

Change to use the non-deprecated APIs, which should work. I wasn't able
to reproduce this locally; I assume it's memory dependent.

Refs: nodejs/node#42714
Fixes: nodejs/node#44143
PR-URL: nodejs/node#44144
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
flaky-test Issues and PRs related to the tests with unstable failures on the CI. net Issues and PRs related to the net subsystem.
Projects
None yet
3 participants