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

tidb-server: close Domain and Storage before HTTP listener #7558

Closed
wants to merge 1 commit into from

Conversation

coocood
Copy link
Member

@coocood coocood commented Aug 30, 2018

What problem does this PR solve?

If the tidb-server failed to quit, we need to know why.
But HTTP listener is closed before domain and storage, we can get the goroutine stack info.
It can be helpful to solve #7545

What is changed and how it works?

add SetCleanupFunc method to Server, call this function in Close before HTTP listener is closed.

Check List

Tests

  • Manual test (add detailed scripts or steps below)
  1. Add Sleep(time.Munite) in cleanup.
  2. make server
  3. start the server
  4. kill the server.
  5. try to connect the server with mysql-client, should be failed.
  6. visit http://localhost:10080/debug/pprof, we can see the page.

Code changes

  • Has exported function/method change

@coocood
Copy link
Member Author

coocood commented Aug 31, 2018

@crazycs520 @tiancaiamao PTAL

@tiancaiamao
Copy link
Contributor

@coocood
Copy link
Member Author

coocood commented Aug 31, 2018

@tiancaiamao
It works, you can try manual test steps.

@tiancaiamao
Copy link
Contributor

Oh, I see. The exit sequence:

  • get signal
  • signal handle will call server.Close
  • clean up function in server.Close
  • the clean up function will close domain [store and domain close]
  • server.Close close the stats listening handle after clean up is done [http listener close]

LGTM

@tiancaiamao tiancaiamao added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 31, 2018
@lysu
Copy link
Contributor

lysu commented Sep 1, 2018

To get goroutine, how about add a new signal handler(e.g. kill -USR1, in jvm we use kill -3) and then call pprof.Lookup("goroutine") write result to stdout or log at last, instead of close port? 😆

shutdown port first is common, because we don't know whether http api will do some important jobs in furture, close port in last maybe break those jobs during shutdown process logic.

@coocood
Copy link
Member Author

coocood commented Sep 2, 2018

@lysu
Good point!
You can write a PR for it, then this can be closed.

@coocood coocood closed this Sep 3, 2018
@coocood coocood deleted the close-storge-before-http branch September 3, 2018 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/LGT1 Indicates that a PR has LGTM 1.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants