Http CloseHandle for closing while waiting #331

carllin · 2018-10-24T01:45:19Z

Add CloseHandle to Json Http server to allow a different thread to close() after calling wait() on main server thread. Based on similar work for websockets: https://github.com/paritytech/jsonrpc/pull/151/files

…ain server thread

bkchr

Looks good so far. Some minor nitpicks :)

http/src/lib.rs

bkchr · 2018-10-24T08:29:34Z

http/src/lib.rs

+
+/// A handle that allows closing of a server even if it owned by a thread blocked in `wait`.
+pub struct CloseHandle {
+    close: Arc<Mutex<Option<Vec<oneshot::Sender<()>>>>>,


More whitespaces.

bkchr · 2018-10-24T08:38:01Z

http/src/lib.rs

 }
+
+/// A handle that allows closing of a server even if it owned by a thread blocked in `wait`.
+pub struct CloseHandle {


I think we should implement/derive Clone for the CloseHandle.

futures::Sender, doesn't implement Clone(), so how should we approach this?

Do you mean the executor_close can not be cloned?

@carllin it's wrapped in Arc<Mutex anyway, so it doesn't really matter. If derive doesn't work then you need to implement Clone manually (and just clone the Arc)

@bkchr, @tomusdrw, yes the executor_close cannot be cloned.

So let's wrap it into Arc as well. I also don't quite understand why do we need to close the executor when we close the server? It wasn't the case for WS or IPC server?

@tomusdrw, in the current WS implementation, close() also closes the executor: https://github.com/paritytech/jsonrpc/blob/master/ws/src/server.rs#L149

tomusdrw

Looks good, please add missing docs and re-format the code using tabs (see .editorconfig file).
Could you also explain why do we need close_handle for Executor and RpcEventLoop?

server-utils/src/reactor.rs

carllin · 2018-10-24T19:01:21Z

@tomusdrw I added the close_handle in Executor and RpcEventLoop because I wanted to support the following behavior:

Grab the CloseHandle for the Server
Call Wait() on the Server
Call CloseHandle.Close()

Unfortunately, if the CloseHandle just held a reference to a lock to the Executors (as was done in the websockets implementation), b/c the Wait() function also simultaneously needs access to the Executors, this won't work. Thus the close_handle() on the reactor and executor return the inner Complete() signaler that executor->close() would have called anyway, so now the CloseHandle no longer needs shared access/lock to the Executor.

carllin · 2018-10-30T18:10:33Z

@tomusdrw, @bkchr, pinging for updates.

tomusdrw · 2018-10-30T18:40:10Z

http/src/lib.rs

+
+	/// Returns a handle to the server that can be used to close it while another thread is
+	/// blocking in `wait`.
+	pub fn close_handle(&mut self) -> CloseHandle {


The function shouldn't mutate anything, so it should be &self.

Suggested change

pub fn close_handle(&mut self) -> CloseHandle {

pub fn close_handle(&self) -> CloseHandle {

let executor_close: Option<Vec<_>> = self.executor.as_ref().map(|executors| {

executors

.iter()

.map(|executor| executor.close_handle())

.collect()

});

...

}

tomusdrw · 2018-10-30T18:41:18Z

server-utils/src/reactor.rs

 	}

+	/// Returns a handle to close the underlying event loop
+	pub fn close_handle(&mut self) -> Option<futures::Complete<()>> {


mut not needed

Suggested change

pub fn close_handle(&mut self) -> Option<futures::Complete<()>> {

pub fn close_handle(&self) -> Option<futures::Complete<()>> {

tomusdrw · 2018-10-30T18:46:00Z

server-utils/src/reactor.rs

+
+	/// Returns the close signal that can be used to close the event loop even while
+	/// another thread is blocked on the event loop in "wait"
+	pub fn close_handle(&mut self) -> Option<futures::Complete<()>>{


This function doesn't make much sense - the whole point of handle is to be able to have multiple instance of it.
This function here will take() the close handle and only return it for the first caller. Subsequent calls will always yield None.
What's more it violates the assumption in the rest of this struct (see expect("..") calls) - so every other function like close() or wait() will actually panic.

I still believe that having this function is completely unnecessary. See how it's done in WebSockets server - we own Executor via Arc<Mutex<Option<Executor>>> and close it completely inside CloseHandle (actually it's not perfect there, cause cloning the handle and calling close() on a clone would panic, but it's a separate issue).

tomusdrw · 2018-10-30T18:51:12Z

http/src/lib.rs

 /// jsonrpc http server instance
 pub struct Server {
 	address: SocketAddr,
 	executor: Option<Vec<Executor>>,


executor should most likely be wrapped in Arc<Mutex< as wel, so that it can be passed to CloseHandle. Actually it would be simpler to store CloseHandle directly in Server and just clone it when requested. CloseHandle should handle the case if Options are None.
To simplify it further you can just have:

pub struct Server { address: SocketAddr, close_handle: CloseHandle, } impl Server { ... pub fn close_handle(&self) -> CloseHandle { self.close_handle.clone() } } #[derive(Clone)] pub struct CloseHandle { handle: Arc<Mutex<Option<(Executor, Vec<oneshot::Sender<()>)>>>, } impl CloseHandle { /// <docs ...> /// returns `false` if the server was closed earlier pub fn close(self) -> bool { if let Some((executor, senders)) = self.handle.lock().take() { executor.close(); for s in senders { s.send(()).expect(...) } true } else { false } } }

Not sure how this would work: once the server starts waiting it has to take the lock on the handle, so close() as written above will hang on the lock?

carllin · 2018-11-05T08:51:46Z

@tomusdrw The WebSockets server has a separate handle that it joins on in wait(), whereas the wait() implementation in this module requires access to the executor, so we wouldn't be able to call close() on the close handle while another thread is blocked on wait(). Is there a way to construct a similar, separate handle as exists in the WebSockets implementation?

tomusdrw · 2018-11-05T09:07:05Z

@carllin Indeed WebSockets server runs an extra thread for itself, and in close handle we wait for that thread to finish. With HTTP the proper solution would be to add another oneshot future that is triggered after the serve is finished here:

jsonrpc/http/src/lib.rs

Line 461 in 2ed142d

executor.spawn(future::lazy(move || {

And Server::wait() should actually wait for this, not the entire excutor to be closed.

tomusdrw · 2018-12-07T09:23:01Z

@carlin I'd like to get that merged, did you consider implementing my proposed changes? We definitely need to support multiple CloseHandles`, that's the whole point of having a clonable handle - otherwise only one handle can be present, cause others will just panic.

carllin · 2018-12-07T18:36:46Z

Hey Tomas, Sorry I haven't been able to get to it, I've been swamped with work at work xD. Hopefully I might have some time closer to vacation time. Thanks for following up! Best, Carl

…

On Fri, Dec 7, 2018 at 1:23 AM Tomasz Drwięga ***@***.***> wrote: @carlin <https://github.com/carlin> I'd like to get that merged, did you consider implementing my proposed changes? We definitely need to support multiple CloseHandles`, that's the whole point of having a clonable handle - otherwise only one handle can be present, cause others will just panic. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#331 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADN-z91hK-8FdZLPjPTgGgyaQG19Q-8aks5u2jN2gaJpZM4X2-Eu> .

dvdplm · 2019-05-24T18:35:59Z

@carllin any chance we can get the changes in? If things have piled up on your end we can perhaps take over and finish it up, just let us know! :)

carllin · 2019-05-24T23:53:19Z

@dvdplm, sorry don't have much bandwidth nowadays xD. Of somebody could drag this to the finish line that would be highly appreciated :D. Thanks for all the help!

dvdplm · 2019-05-28T08:00:09Z

Closing in favour of #437

Add ability to call close on another thread after calling wait() on m…

e61797a

…ain server thread

bkchr requested changes Oct 24, 2018

View reviewed changes

tomusdrw reviewed Oct 24, 2018

View reviewed changes

server-utils/src/reactor.rs Show resolved Hide resolved

server-utils/src/reactor.rs Show resolved Hide resolved

server-utils/src/reactor.rs Show resolved Hide resolved

Change spaces to tabs, add missing documentation

c4bc260

carllin force-pushed the master branch from de62323 to c4bc260 Compare October 25, 2018 06:33

Make CloseHandle cloneable

a08a0c8

tomusdrw suggested changes Oct 30, 2018

View reviewed changes

CriesofCarrots mentioned this pull request Nov 5, 2018

Return to paritytech/jsonrpc solana-labs/solana#1706

Closed

dvdplm mentioned this pull request May 25, 2019

Add a way to close the http server while waiting #437

Merged

dvdplm closed this May 28, 2019

-	pub fn close_handle(&mut self) -> CloseHandle {
+	pub fn close_handle(&self) -> CloseHandle {
+	  let executor_close: Option<Vec<_>> = self.executor.as_ref().map(|executors| {
+			executors
+				.iter()
+				.map(|executor| executor.close_handle())
+				.collect()
+		});
+		...
+	}

	pub fn close_handle(&mut self) -> Option<futures::Complete<()>> {
	pub fn close_handle(&self) -> Option<futures::Complete<()>> {

Http CloseHandle for closing while waiting #331

Http CloseHandle for closing while waiting #331

Uh oh!

Conversation

carllin commented Oct 24, 2018

Uh oh!

bkchr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkchr Oct 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomusdrw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

carllin commented Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carllin commented Oct 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomusdrw Oct 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carllin commented Nov 5, 2018

Uh oh!

tomusdrw commented Nov 5, 2018

Uh oh!

tomusdrw commented Dec 7, 2018

Uh oh!

carllin commented Dec 7, 2018 via email

Uh oh!

dvdplm commented May 24, 2019

Uh oh!

carllin commented May 24, 2019

Uh oh!

dvdplm commented May 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bkchr Oct 25, 2018 •

edited

Loading

carllin commented Oct 24, 2018 •

edited

Loading

tomusdrw Oct 30, 2018 •

edited

Loading