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

admin.StopRPC added to console #734

Merged
merged 4 commits into from
Apr 20, 2015
Merged

admin.StopRPC added to console #734

merged 4 commits into from
Apr 20, 2015

Conversation

bas-vk
Copy link
Member

@bas-vk bas-vk commented Apr 17, 2015

Adds support for stopping the rpc service in the console, this closes #729.

  1. When the user issues a stopRPC command the listener is stopped immediately and new connections are not accepted.
  2. Active connections (http keep-alive) follow the go principe and run in their own goroutines. The net package doesn't provide an easy way to interact with these routines. Therefore the ClosableConnection type was added. This is a net.TCPConn but with a specialised Read method. This method tests if the listener was stopped, in that case it returns an io.EOF. If not stopped it cals the TCPConn.Read method.
  3. The net package would still call the handler in case of an read error before it stops. Therefore the NewStoppableHandler is added. This wraps the default json/cors handlers and does a check if the listener was stopped. In that case it would return an error indicating that the RPC service is stopped. After that the connection is closed.

Note, it doesn't do a graceful shutdown. This could be added but will stall the console while waiting for idle connections to timeout.

@fjl
Copy link
Contributor

fjl commented Apr 17, 2015

As general feedback, there is no need to store pointers to channels. Just store the channel directly.

@fjl
Copy link
Contributor

fjl commented Apr 17, 2015

What exactly is so bad about just closing the listener and letting those connections time out? Given that stopRPC isn't something most people will ever use, it doesn't really warrant the huge complexity.

@bas-vk
Copy link
Member Author

bas-vk commented Apr 17, 2015

Not closing connections can cause unexpected behaviour from the users point of view. If you let these connections remain open users will be able to execute transactions as long as the connection is kept alive which is not really what you would expected after a stopRPC.

Closing idle connection will (hopefully) be supported in go 1.5. What I suggest, support closing idle connections for now and when golang/go/issues/4674 makes it to go 1.5 and most of our users are using go 1.5 replace this code with the code from the package.

@bas-vk
Copy link
Member Author

bas-vk commented Apr 17, 2015

Just had a quick chat with @obscuren, we use the PR as is and when the CloseIdeConnections is implemented in the net package we remove this code. I will make add a comment that this code should be removed in the future.

@fjl
Copy link
Contributor

fjl commented Apr 17, 2015

                                                                                  Maybe it should not be exported.

@zelig
Copy link
Contributor

zelig commented Apr 20, 2015

this can merge if green

obscuren added a commit that referenced this pull request Apr 20, 2015
admin.StopRPC added to console
@obscuren obscuren merged commit 99e825a into ethereum:develop Apr 20, 2015
@bas-vk bas-vk deleted the issue-729 branch April 22, 2015 10:51
ngtuna pushed a commit to ngtuna/tomochain that referenced this pull request Sep 26, 2019
maoueh pushed a commit to streamingfast/go-ethereum that referenced this pull request Jun 9, 2023
Make txArriveTimeout Configurable w/ CLI Flag
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.

4 participants