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

Server doesn't shut down with Ctrl+C #2875

Closed
kulshekhar opened this issue Oct 16, 2016 · 17 comments · Fixed by #2964
Closed

Server doesn't shut down with Ctrl+C #2875

kulshekhar opened this issue Oct 16, 2016 · 17 comments · Fixed by #2964
Labels
type:question Support or code-level question

Comments

@kulshekhar
Copy link
Contributor

kulshekhar commented Oct 16, 2016

When using parse server with the Postgres database adapter, the server doesn't shut down when CTRL+C is pressed.

Steps to reproduce

  1. Start parse server:

    parse-server --appId a --masterKey m --databaseURI postgres://username:password@localhost:5432/db-name
    
  2. Start parse dashboard and point it to this

    parse-dashboard --appId a --masterKey m --serverURL http://localhost:1337/parse
    
  3. Open the parse dashboard in a browser (http://localhost:4040/apps)

  4. Stop parse dashboard (this step isn't necessary)

  5. Try to stop parse server by pressing CTRL+C in the terminal where it was started.

Expected Results

Parse server stops

Actual Outcome

The following message is displayed and the server hangs:

Termination signal received. Shutting down.

If CTRL+C is pressed repeatedly (about 10-12 times), the following message is displayed:

(node:32165) Warning: Possible EventEmitter memory leak detected. 11 close listeners added. Use emitter.setMaxListeners() to increase limit

Note that if parse dashboard isn't used, CTRL+C works as expected and parse server shuts down.

Environment Setup

  • Server
    • parse-server version : commit de36d96 on master
    • Operating System: Linux Mint 17.3
    • Hardware: amd64
    • Localhost or remote server? localhost

Note: this issue is also present when using the release 2.2.22. However, with this release, some code has to be modified to use the Postgres adapter.

  • Database
    • Postgres version: 9.5/9.6
    • MongoDB: 3.2.10
    • Hardware: linux amd64
    • Localhost or remote server? localhost

Logs/Trace

appId: a
masterKey: ***REDACTED***
port: 1337
databaseURI: postgres://username:password@localhost:5432/parse-test
mountPath: /parse
maxUploadSize: 20mb
verbose: 1
serverURL: http://localhost:1337/parse

[32517] parse-server running on http://localhost:1337/parse
verbose: REQUEST for [GET] /parse/serverInfo: {} method=GET, url=/parse/serverInfo, host=localhost:1337, connection=keep-alive, content-length=140, origin=http://localhost:4040, user-agent=Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.59 Safari/537.36, content-type=text/plain, accept=*/*, referer=http://localhost:4040/apps, accept-encoding=gzip, deflate, br, accept-language=en-US,en;q=0.8, 
verbose: RESPONSE from [GET] /parse/serverInfo: {
  "response": {
    "features": {
      "globalConfig": {
        "create": true,
        "read": true,
        "update": true,
        "delete": true
      },
      "hooks": {
        "create": false,
        "read": false,
        "update": false,
        "delete": false
      },
      "cloudCode": {
        "jobs": true
      },
      "logs": {
        "level": true,
        "size": true,
        "order": true,
        "until": true,
        "from": true
      },
      "push": {
        "immediatePush": true,
        "scheduledPush": false,
        "storedPushData": true,
        "pushAudiences": false
      },
      "schemas": {
        "addField": true,
        "removeField": true,
        "addClass": true,
        "removeClass": true,
        "clearAllDataFromClass": true,
        "exportClass": false,
        "editClassLevelPermissions": true,
        "editPointerPermissions": true
      }
    },
    "parseServerVersion": "2.2.22"
  }
} create=true, read=true, update=true, delete=true, create=false, read=false, update=false, delete=false, jobs=true, level=true, size=true, order=true, until=true, from=true, immediatePush=true, scheduledPush=false, storedPushData=true, pushAudiences=false, addField=true, removeField=true, addClass=true, removeClass=true, clearAllDataFromClass=true, exportClass=false, editClassLevelPermissions=true, editPointerPermissions=true, parseServerVersion=2.2.22
verbose: REQUEST for [GET] /parse/classes/_Installation: {
  "where": {},
  "limit": 0,
  "count": 1
} method=GET, url=/parse/classes/_Installation, host=localhost:1337, connection=keep-alive, content-length=171, origin=http://localhost:4040, user-agent=Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.59 Safari/537.36, content-type=text/plain, accept=*/*, referer=http://localhost:4040/apps, accept-encoding=gzip, deflate, br, accept-language=en-US,en;q=0.8, , limit=0, count=1
verbose: REQUEST for [GET] /parse/classes/_User: {
  "where": {},
  "limit": 0,
  "count": 1
} method=GET, url=/parse/classes/_User, host=localhost:1337, connection=keep-alive, content-length=171, origin=http://localhost:4040, user-agent=Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.59 Safari/537.36, content-type=text/plain, accept=*/*, referer=http://localhost:4040/apps, accept-encoding=gzip, deflate, br, accept-language=en-US,en;q=0.8, , limit=0, count=1
verbose: RESPONSE from [GET] /parse/classes/_Installation: {
  "response": {
    "results": [],
    "count": 0
  }
} results=[], count=0
verbose: RESPONSE from [GET] /parse/classes/_User: {
  "response": {
    "results": [],
    "count": 0
  }
} results=[], count=0
^CTermination signal received. Shutting down.
^CTermination signal received. Shutting down.
^CTermination signal received. Shutting down.
^CTermination signal received. Shutting down.
^CTermination signal received. Shutting down.
^CTermination signal received. Shutting down.
^CTermination signal received. Shutting down.
^CTermination signal received. Shutting down.
^CTermination signal received. Shutting down.
^CTermination signal received. Shutting down.
^CTermination signal received. Shutting down.
(node:32517) Warning: Possible EventEmitter memory leak detected. 11 close listeners added. Use emitter.setMaxListeners() to increase limit
^CTermination signal received. Shutting down.
@kulshekhar
Copy link
Contributor Author

The cause of this issue is probably causing the current tests with postgres to fail.

https://travis-ci.org/ParsePlatform/parse-server/jobs/171414959

In spec/helper.js

server = app.listen(port);

is throwing an exception most likely because the preceding

server.close()

couldn't close the server.

@flovilmart
Copy link
Contributor

@kulshekhar
Copy link
Contributor Author

My observation in the previous comment was incorrect (the recent PRs are passing all tests)

It was actually the express server which was keeping connections on for the default timeout period. Once those sockets are closed (as done in #2959) this problem goes away

@flovilmart
Copy link
Contributor

flovilmart commented Oct 29, 2016

This is not the express server getting keeping the connections, and if it ever was we'd see that behaviour with mongodb too. Given the documentation of pg-promise, and that we don't call pgp.end() nor client.release() I would expect that to cause this issue.

I believe your PR works because you're also closing the pg connections but that's really not clean.

@kulshekhar
Copy link
Contributor Author

I'll take a closer look at this.

I haven't yet tested this with mongodb but will try to do so. I'll also see if I can add some tests to check against this behaviour.

I've added a note on #2959 to not merge it yet.

@flovilmart
Copy link
Contributor

That doesn't make sense that express would retain those connections, that would mean that everyone using express would have that behaviour... I would look into postgres as I mentioned, the pg-promise library explicitly ask to call pgp.end.

@kulshekhar
Copy link
Contributor Author

kulshekhar commented Oct 29, 2016

Yes. I'll begin with looking at pgp

@flovilmart
Copy link
Contributor

Also, if the issue is really on our express app side, this should not be handled into the CLI, but into the parse-server module itself.

@kulshekhar
Copy link
Contributor Author

I've just tested this with mongodb (fresh install on a fresh VM) and the same issue exists. It doesn't seem to be Postgres specific

@kulshekhar
Copy link
Contributor Author

The connection=keep-alive header is the cause of this issue.

In a way, parse server is actually behaving as it should. The question is should it close all open connections immediately when it receives SIGTERM/SIGINT?

@flovilmart
Copy link
Contributor

I believe the HTTP server should close those connections. But that's odd that we have to manually manage it.

@kulshekhar
Copy link
Contributor Author

I had submitted a PR for this but I've closed that. I'd actually prefer to write tests to test this behavior first.

However, I'm not sure of what the best approach would be. Should the tests spawn parse-server as a child process and test this behavior against that. Or should the code in src/cli/parse-server.js be modified to expose some methods and objects that could be imported and used in tests?

@flovilmart
Copy link
Contributor

In the unit tests, we ensure no connection is left open after each test, which is odd...

I believe your were on the right track can we try something in those likes:

https://github.com/isaacs/server-destroy/blob/master/index.js

@kulshekhar
Copy link
Contributor Author

kulshekhar commented Oct 30, 2016

In the unit tests, we ensure no connection is left open after each test, which is odd...

That's because the requests made in the tests don't use keep-alive. The test would fail if this were added. For instance, if any test in ParseAPI.spec.js was modified like so:

    request.get({
      .
      .
      agent: new require('http').Agent({keepAlive: true})
    }, (error, response, body) => {
      .
      .
      .
    });

the test would fail with the message that there were open connections. (This is the default behavior for browsers so maybe the tests should be modified like this?)

This is apparently a known issue with nodejs nodejs/node#2642

There's also a PR open that attempts to partially fix this nodejs/node#2534. It seems to have been open for a long time, though.

Based on the discussion around this issue in the node repo, it isn't certain if we'll be seeing a fix for this anytime soon.

It does seem to be a bug in node because none of the other servers (nginx, apache, etc) exhibit this behavior. They close all connections when they receive a signal to close.

@kulshekhar kulshekhar changed the title Server doesn't shut down with Ctrl+C when using Postgres Server doesn't shut down with Ctrl+C Oct 30, 2016
@Ashot-KR
Copy link

Ashot-KR commented Apr 27, 2017

I'm still having this issue when using parse server as express middleware. What i'm doing wrong? (MacOS 10.12.4, node v7.9.0)

@flovilmart
Copy link
Contributor

See: #3718

@Ashot-KR
Copy link

@flovilmart missed that issue. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:question Support or code-level question
Projects
None yet
4 participants