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

Added missing conditional at scheduleDestroy function #6

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

UlisesGascon
Copy link
Member

Close #5

@feliun
Copy link

feliun commented Jan 27, 2020

@UlisesGascon maybe a bit more "JS flavour" by doing destroy && destroy.unref() ?

@UlisesGascon
Copy link
Member Author

@feliun I added a better validation destroy instanceof Object now as 0c6d921

@feliun
Copy link

feliun commented Jan 27, 2020

this sounds to me like this the main issue may still happen? do we know why this happens?

@ismaelocaramelo
Copy link

ismaelocaramelo commented Apr 6, 2020

Hi folks!

I was going to open a PR about this, but since it's already done by @UlisesGascon I'll comment here my thoughts. Bear in mind this is still quite new to me, however, I've just encountered this problem and after reading this from Nodejs doc:

When called, the active Immediate object will not require the Node.js event loop to remain active. If there is no other activity keeping the event loop running, the process may exit before the Immediate object's callback is invoked. Calling immediate.unref() multiple times will have no effect.

If we process.exit(0) the .unref will not take effect. And this what I think it's happening, as neither I can not find a reason why unref is used. Calling clearTimeout(destroy) would be much safer/cleaner in case you'd like to keep to avoid any TypeError on the unref thing. However, my suggestion would be more like deleting that line as I believe server.destroy is taking care of it.

@UlisesGascon @feliun

@Betisman
Copy link
Contributor

@UlisesGascon @feliun @ismaelocaramelo I see this PR is open. Can we merge or close it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug at scheduleDestroy
4 participants