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

server, tidb-server: improve unix socket handling #8836

Merged
merged 17 commits into from
Jan 9, 2019
Merged

server, tidb-server: improve unix socket handling #8836

merged 17 commits into from
Jan 9, 2019

Conversation

morgo
Copy link
Contributor

@morgo morgo commented Dec 26, 2018 •
edited by c4pt0r
Loading

What problem does this PR solve?

Fixes #8459

What is changed and how it works?

  • Allows server to listen on both unix socket and TCP at the same time
  • Removes socket file on server clean exit
  • Handles the error / refuses to start if the socket file is already in use.
  • Supports the case that neither a socket file or tcp (host,port) are specified

Check List

Tests

  • Manual test

I'm not actually sure how to write tests to validate it, but happy to do so with pointers.

Code changes

  • Has exported function/method change

Side effects

  • Possible performance regression (socket access may be slower)
  • Breaking backward compatibility (socket access when both tcp and socket enabled will authenticate like tcp connections)
  • Increased code complexity

Related changes

  • Need to be included in the release note

This change is Reviewable

@morgo
server, tidb-server: improve socket handling
08320d3 
Cleanup socket file on exit
Handle error of socket file exists
Support both socket and tcp via redirection
Support case neither socket or tcp specified
@morgo
server: fixed linter, fixed test to use sock again
4f956a9
@morgo morgo requested a review from jackysp December 26, 2018 19:43
@morgo morgo requested a review from coocood December 26, 2018 19:44
@morgo
Copy link
Contributor Author

morgo commented Dec 26, 2018

PTAL @coocood, @jackysp. Thx!

@morgo
Copy link
Contributor Author

morgo commented Dec 26, 2018

/run-all-tests

@morgo morgo changed the title server, *: improve unix socket handling server, tidb-server: improve unix socket handling Dec 26, 2018
@morgo
Copy link
Contributor Author

morgo commented Dec 26, 2018

/run-all-tests

@jackysp
Copy link
Member

jackysp commented Dec 27, 2018

PTAL @tiancaiamao

gregwebs
gregwebs reviewed Dec 28, 2018
View reviewed changes
Copy link
Contributor

@gregwebs gregwebs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a good idea.

server/server.go Show resolved Hide resolved
server/server.go Outdated Show resolved Hide resolved
tidb-server/main.go Outdated Show resolved Hide resolved
server/server.go Outdated Show resolved Hide resolved
server/server.go Outdated Show resolved Hide resolved
server/server.go Outdated Show resolved Hide resolved
server/server.go
}
}

func (s *Server) handleForwardedConnection(uconn net.Conn, addr string) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than logging warnings, I would return an error and have the caller deal with it (the caller can log or retry).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, handleForwardedConnection is an async call (called as a go routine), so I think it is important to handle its own?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, the caller would expand to an anonymous function that calls this function and does something with the error.

coocood
coocood reviewed Dec 28, 2018
View reviewed changes
server/server.go Outdated Show resolved Hide resolved
coocood
coocood reviewed Dec 28, 2018
View reviewed changes
server/server.go Outdated Show resolved Hide resolved
@morgo
Copy link
Contributor Author

morgo commented Dec 28, 2018

I am not sure if I tested it wrong yesterday, but I can't seem to get the server to listen on tcp + socket. I tried:

make server && ./bin/tidb-server -socket /tmp/tidb.sock &
mysql --no-defaults -S /tmp/tidb.sock

The mysql client just hangs until I control + c, then get the error:

ERROR 2013 (HY000): Lost connection to MySQL server at 'reading initial communication packet', system error: 95

I appreciate any feedback if you know what's wrong :(

@coocood
Copy link
Member

coocood commented Dec 28, 2018

The listener seems still not closed in Server.Close method.

@morgo morgo removed the status/DNM label Dec 29, 2018
@morgo morgo mentioned this pull request Jan 3, 2019
Merged
coocood
coocood reviewed Jan 3, 2019
View reviewed changes
server/tidb_test.go Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Jan 3, 2019 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@78a51a4). Click here to learn what that means.
The diff coverage is 69.44%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #8836   +/-   ##
=========================================
  Coverage          ?   67.53%           
=========================================
  Files             ?      363           
  Lines             ?    75142           
  Branches          ?        0           
=========================================
  Hits              ?    50746           
  Misses            ?    19918           
  Partials          ?     4478
Impacted Files Coverage Δ
server/server.go 56.85% <69.44%> (ø)

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 78a51a4...207900a. Read the comment docs.

@coocood
Copy link
Member

coocood commented Jan 7, 2019

LGTM

@morgo morgo added the status/LGT1 Indicates that a PR has LGTM 1. label Jan 7, 2019
@jackysp
Copy link
Member

jackysp commented Jan 8, 2019

/run-all-tests

@morgo
Copy link
Contributor Author

morgo commented Jan 8, 2019

/run-all-tests

@jackysp
Copy link
Member

jackysp commented Jan 9, 2019

/run-unit-test

jackysp
jackysp approved these changes Jan 9, 2019
View reviewed changes
Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jackysp jackysp merged commit 692693a into pingcap:master Jan 9, 2019
@morgo morgo deleted the socket branch January 9, 2019 02:32
@morgo morgo mentioned this pull request Feb 8, 2019
Closed
@morgo morgo mentioned this pull request Mar 6, 2019
Closed
5 tasks
@sre-bot sre-bot added the contribution This PR is from a community contributor. label Dec 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coocood coocood coocood left review comments

@gregwebs gregwebs gregwebs left review comments

@jackysp jackysp jackysp approved these changes

@tiancaiamao tiancaiamao Awaiting requested review from tiancaiamao

Assignees
No one assigned
Labels
component/server contribution This PR is from a community contributor. status/LGT1 Indicates that a PR has LGTM 1. type/compatibility
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

tidb-server options -socket and -host are incompatible
6 participants
@morgo @jackysp @coocood @codecov-io @gregwebs @sre-bot