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

REST API Server Need Support Stop #1934

Open
garyyu opened this issue Nov 6, 2018 · 7 comments

Comments

@garyyu
Copy link
Member

commented Nov 6, 2018

Our REST API server doesn't support stop like other servers, which cause an annoying Address already in use problem when I want to stop and restart a server (in a test case which is to be added for fork syncing test and so on).

api::start_rest_apis(
config.api_http_addr.clone(),
shared_chain.clone(),
tx_pool.clone(),
p2p_server.peers.clone(),
api_secret,
None,
);

apis.start(socket_addr, router, tls_config).is_ok()

https://github.com/mimblewimble/grin/blob/master/api/src/rest.rs#L161-L173

	/// Starts ApiServer at the provided address.
	/// TODO support stop operation
	pub fn start(
		&mut self,
		addr: SocketAddr,
		router: Router,
		conf: Option<TLSConfig>,
	) -> Result<thread::JoinHandle<()>, Error> {
		match conf {
			Some(conf) => self.start_tls(addr, router, conf),
			None => self.start_no_tls(addr, router),
		}
	}

Who would like to help to add the stop support?
just like

stop.clone(),
,
stop.clone(),
,
stop.clone(),

and so on.

@hashmap

This comment has been minimized.

Copy link
Member

commented Nov 6, 2018

Code for http server is in place, there was an issue with hyper, I’ll check if it was resolved. TLS part is not implemented yet.

@sesam

This comment has been minimized.

Copy link
Contributor

commented Nov 6, 2018

I know the pain of hunting processes with open ports. As I understand @garyyu it's probably necessary to bring along the stop: Arc into start_rest_apis

@sesam

This comment has been minimized.

Copy link
Contributor

commented Nov 6, 2018

There's a disabled stop() here:

// TODO re-enable stop after investigation

which is not far below the TODO about adding stopping support. :-) Maybe still adding stop support differently might be useful?

@garyyu garyyu referenced this issue Nov 7, 2018
@kargakis

This comment has been minimized.

Copy link
Contributor

commented Apr 7, 2019

Address already in use looks like a port was leaked which is a different issue from supporting stop. We should ensure everything is cleaned up / gracefully terminated when grin receives a termination signal rather than adding the ability to stop grin which doesn't solve the underlying issue.

EDIT: Maybe a duplicate of #2364

@kargakis

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

Graceful shutdown is added at #2812.

I think we can close this issue now.

@hashmap

This comment has been minimized.

Copy link
Member

commented May 17, 2019

Not yet, rest server internally doesn't try to stop gracefully because of a Hyper issue. It's in my todo list, as well as stop of stratum server.

@hashmap hashmap reopened this May 17, 2019

@hashmap

This comment has been minimized.

Copy link
Member

commented Jun 14, 2019

Will take care of it

@hashmap hashmap self-assigned this Jun 14, 2019

@quentinlesceller quentinlesceller referenced this issue Jul 23, 2019
1 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.