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.

Sign up for GitHub

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

Jump to bottom

CITGM fail #1118

Closed
refack opened this issue May 22, 2017 · 7 comments
Closed

CITGM fail #1118

refack opened this issue May 22, 2017 · 7 comments

Comments

@refack
Copy link

refack commented May 22, 2017

Maybe just a load induced timeout...

 274 passing (5s)
   1 failing
   1) WebSocket permessage-deflate #terminate will raise error callback, if any, if called during send data:
      Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.

Same error on osx1010 and aix61-ppc64
Test: https://github.com/websockets/ws/blob/master/test/WebSocket.test.js#L1990
CITGM report: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/813/testReport/
@refack
Copy link
Author

refack commented May 22, 2017

This is the diff since last passing nodejs/node@171a43a...ad4765a
Prbly nodejs/node@330c8d7

@refack refack mentioned this issue May 22, 2017
Closed
4 tasks
@mcollina
Copy link
Contributor

@refack I do not think opening a bug on the target module is the best approach for core. You should really open a bug in core.

@refack
Copy link
Author

refack commented May 22, 2017

@refack I do not think opening a bug on the target module is the best approach for core. You should really open a bug in core.

I usually do, initially I thought it was a load induced timeout.
Now @lpinca seems to agree nodejs/node#12828 (comment)

P.S. It's CITGM policy to inform module authors https://github.com/nodejs/citgm/blob/master/CONTRIBUTING.md#submitting-a-module-to-citgm

@mcollina
Copy link
Contributor

Anyway, as the author of the offending issue I would have loved to know :).
I'm a bit unsure how I missed that, and why it fails only on AIX and OSX, but not everywhere else.

@refack refack mentioned this issue May 22, 2017
Closed
4 tasks
@refack
Copy link
Author

refack commented May 22, 2017

Anyway, as the author of the offending issue I would have loved to know :).
I'm a bit unsure how I missed that, and why it fails only on AIX and OSX, but not everywhere else.

Big Ack 🤦‍♂️
Re: nodejs/node#12925 (comment)

@mcollina
Copy link
Contributor

Anyway, it's confirmed and a fix is on its way.

@refack refack mentioned this issue May 22, 2017
Closed
@mcollina mcollina mentioned this issue May 22, 2017
Closed
3 tasks
@lpinca
Copy link
Member

lpinca commented May 22, 2017

Awesome, thanks.

mcollina added a commit to mcollina/node that referenced this issue May 22, 2017
@mcollina
stream: fix destroy(err, cb) regression
17aebdf 
Fixed a regression that caused the callback passed to destroy()
to not be called if the stream was already destroyed.
This caused a regression on the ws module in CITGM introduced by
nodejs#12925.

Fixes: websockets/ws#1118
@refack refack closed this as completed May 23, 2017
mcollina added a commit to nodejs/node that referenced this issue May 24, 2017
@mcollina
stream: fix destroy(err, cb) regression
ccd3ead 
Fixed a regression that caused the callback passed to destroy()
to not be called if the stream was already destroyed.
This caused a regression on the ws module in CITGM introduced by
#12925.

PR-URL: #13156
Fixes: websockets/ws#1118
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Calvin Metcalf <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
jasnell pushed a commit to nodejs/node that referenced this issue May 24, 2017
@mcollina @jasnell
stream: fix destroy(err, cb) regression
8990726 
Fixed a regression that caused the callback passed to destroy()
to not be called if the stream was already destroyed.
This caused a regression on the ws module in CITGM introduced by
#12925.

PR-URL: #13156
Fixes: websockets/ws#1118
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Calvin Metcalf <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
jasnell pushed a commit to nodejs/node that referenced this issue May 28, 2017
@mcollina @jasnell
stream: fix destroy(err, cb) regression
87cef63 
Fixed a regression that caused the callback passed to destroy()
to not be called if the stream was already destroyed.
This caused a regression on the ws module in CITGM introduced by
#12925.

PR-URL: #13156
Fixes: websockets/ws#1118
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Calvin Metcalf <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@BridgeAR BridgeAR mentioned this issue May 20, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@mcollina @refack @lpinca