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

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

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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