Skip to content

Conversation

@faern
Copy link
Contributor

@faern faern commented Jun 8, 2017

Solves #148

@faern
Copy link
Contributor Author

faern commented Jun 8, 2017

If someone knows a way to do this without a mutex that would of course be nicer, but this works.

@faern faern force-pushed the ws-close-handle branch from 8c47e03 to e5da3f0 Compare June 8, 2017 13:50
@tomusdrw
Copy link
Contributor

tomusdrw commented Jun 8, 2017

You could try to make server-utils::reactor::Remote cloneable and move the Mutex there, cause it's only required if the event loop has actually been spawned.

@faern
Copy link
Contributor Author

faern commented Jun 9, 2017

@tomusdrw I looked into moving the locking now. However, I think it made things less clear/nice. The variant would become Spawned(Arc<Mutex<Option<RpcEventLoop>>>). The option is there since RpcEventLoop::close and wait consumes self, so I must take the ownership of it. And it resulted in having to lock the mutex in all three methods on Remote.

The current solution put the lock higher up, but the only place where it's actually locked is in CloseHandle::close. So there is no locking performance penalty (except the Arc decrement) unless someone actually use the CloseHandle, and since closing is a one time operation it should not be that crucial.

Copy link
Contributor

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

Fair enough, lgtm then!

@faern
Copy link
Contributor Author

faern commented Jun 13, 2017

Ping? :)

@tomusdrw tomusdrw merged commit 1b86aaa into paritytech:master Jun 14, 2017
@tomusdrw
Copy link
Contributor

Thanks!

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.

2 participants