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

Remove sigkill and sigint handler added to overcome a long closed issue with node #4635

Closed
wants to merge 3 commits into from

Conversation

acinader
Copy link
Contributor

@acinader acinader commented Mar 12, 2018

fixes: #4593

rip out all of the configureListener code since nodejs/node#2642 has been long resolved.

I have confirmed that when parse server receives sigkill or sigint and the server closes, connections to mongo are also closed.

This removes some questionable code, which I can elaborate on if anyone is curious....

fixes: 4593

I think that this is proper hygiene
Wait a tick before process exit, not for any particular reason than it seems defensive?
@codecov
Copy link

codecov bot commented Mar 12, 2018

Codecov Report

Merging #4635 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4635      +/-   ##
==========================================
- Coverage   92.64%   92.62%   -0.03%     
==========================================
  Files         119      119              
  Lines        8567     8567              
==========================================
- Hits         7937     7935       -2     
- Misses        630      632       +2
Impacted Files Coverage Δ
src/ParseServer.js 96.8% <ø> (ø) ⬆️
src/Adapters/Cache/InMemoryCache.js 91.66% <0%> (-8.34%) ⬇️
...dapters/Storage/Postgres/PostgresStorageAdapter.js 97.11% <0%> (-0.1%) ⬇️
src/Routers/PushRouter.js 96.42% <0%> (+3.57%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a323001...b1c09ce. Read the comment docs.

@dplewis
Copy link
Member

dplewis commented Mar 16, 2018

I think that this is proper hygiene? Wait a tick before process exit, not for any particular reason other than it seems defensive?

I think this is better than nextTick. Maybe not needed but I like defensive programming.

I can rip out all of the configureListener code since nodejs/node#2642 has been long resolved (i've tested and it shutsdown all nice without that code now.).

Does #2 also solves #3 without calling close()?

…/node#2642 has been fixed so server will just do the right thing now.
@acinader acinader changed the title Tell the process to exit once our shutdown hook has run. Remove sigkill and sigint handler added to overcome a long closed issue with node Mar 16, 2018
@acinader
Copy link
Contributor Author

excellent question @dplewis!

Yes, if I just rip it all out, it just works as I'd expect. So what do I expect? I expect that when I start my server I can see the connection count on the mongo server increase. When I then kill the server with ctrl-c or with kill $PARSE_PIDI see the connection decrease to the level it was at before I started the server.

So I think that the right answer is to just rip it all out. Pr update coming.

Am I missing something?

@flovilmart
Copy link
Contributor

Some adapters still need the handleShutdown IIRC

@flovilmart
Copy link
Contributor

@acinader we actually need the shutdown handler here as some external adapters are expecting it to be called to do some cleanup.

Removing it is a bad idea :) also the liveQuery should register it’s own shutdown handler if that’s the issue at hand

@acinader
Copy link
Contributor Author

k will go back to the drawing board and figure out how to get the livequery server to shutdown.

@flovilmart
Copy link
Contributor

Probably in a similar way to the parse server. Is it when it’s run stand alone that it’s not shutting down? Do you have a code example?

@acinader
Copy link
Contributor Author

acinader commented Mar 16, 2018

@flovilmart this is just when I am running the livequery server in the same process as parse-server which I do for development.

My config:

"liveQuery": {
    "classNames": ["Test"]
  },
  "liveQueryServerOptions": {
    "appId": "...",
    "masterKey": "...",
    "serverURL": "http://localhost:1337/parse",
    "port": "8082",
    "keyPairs": {
      "clientKey": "...",
      "masterKey": "..."
    }
  }

I'm starting the server with npm start and here's what the start script in my package.json looks like:

"scripts": {
    "pretest": "mongodb-runner start",
    "posttest": "mongodb-runner stop",
    "test": "NODE_ENV=test TESTING=1 jasmine",
    "start": "parse-server ./config.json",
    "start-local": "parse-server ./config-local.json"
  },

It makes sense to me that the server is never shutting down since nothing tells the live-query server to stop. The live query server does not have a stop or shutdown method and I've played around a little with how to add one, but I haven't figured out yet. I'm sure I can figure it out with some more time and reading. But I'm trying to understand why it would be necessary and if it's really necessary.

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

Successfully merging this pull request may close these issues.

Live Query Server does not shut down properly
3 participants