Port enclave-runner to tokio 0.2 #226
Conversation
enclave-runner/src/library.rs
Outdated
| @@ -68,13 +71,13 @@ impl Library { | |||
| /// The caller must ensure that the parameters passed-in match what the | |||
| /// enclave is expecting. | |||
| pub unsafe fn call( | |||
| &self, | |||
| &mut self, | |||
There was a problem hiding this comment.
This is a public API change, so we need a minor version bump
There was a problem hiding this comment.
Are there docs somewhere (maybe the EDP website?) that also need to be updated?
There was a problem hiding this comment.
This API change is not appropriate. Multiple threads should be able to call into the enclave.
There was a problem hiding this comment.
Good point, I'll find a way to avoid this change
enclave-runner/src/usercalls/mod.rs
Outdated
| }) | ||
| } | ||
| } | ||
|
|
||
| impl<S: AsyncRead + AsyncWrite + Sync + Send + 'static> AsyncStream for S {} | ||
|
|
||
| /// AsyncListener lets an implementation implement a slightly modified form of `std::net::TcpListener::accept`. | ||
| pub trait AsyncListener: 'static + Send { |
There was a problem hiding this comment.
Why remove 'static?
mzohreva
force-pushed
the
port-to-tokio-0.2
branch
from
f80afa1
to
bb5bf68
Compare
| @@ -1226,18 +1216,7 @@ impl<'tcs> IOHandlerInput<'tcs> { | |||
| return Ok(self.alloc_fd(AsyncFileDesc::stream(stream_ext)).await); | |||
| } | |||
|
|
|||
| // try to connect to all socket addresses one by one | |||
There was a problem hiding this comment.
This is now unnecessary since tokio::net::TcpStream::connect() takes care of it in tokio 0.2 (similar to std::net::TcpStream)
There was a problem hiding this comment.
Wow is there anything tokio 0.2 doesn't do now?
mzohreva
force-pushed
the
port-to-tokio-0.2
branch
from
bb5bf68
to
7aa574d
Compare
enclave-runner/src/usercalls/mod.rs
Outdated
| Async::Ready(v) | ||
| } | ||
| } | ||
| self.poll_read(cx, v.as_mut_slice()).map(move |size| { |
There was a problem hiding this comment.
Maybe rename the |size| to |ready| or |result|
nested map seems awkward if both are called size.
enclave-runner/src/usercalls/mod.rs
Outdated
| @@ -148,7 +124,7 @@ impl Read for Stdin { | |||
| } | |||
|
|
|||
| lazy_static::lazy_static! { | |||
| static ref STDIN: Lock<AsyncStdin> = { | |||
| static ref STDIN: futures::lock::Mutex<AsyncStdin> = { | |||
There was a problem hiding this comment.
Is there a reason to use futures::lock::Mutex instead of the tokio Mutex which is already imported using use declaration?
I think we can choose one and stick with it
There was a problem hiding this comment.
We need to use futures::lock::Mutex in some case because it allows calling lock() for T: ?Sized while tokio's Mutex does not. I replaced the tokio Mutex use with futures one
| @@ -644,8 +634,15 @@ impl EnclaveState { | |||
| ) -> StdResult<(u64, u64), EnclaveAbort<EnclavePanic>> { | |||
| let (tx_return_channel, mut rx_return_channel) = tokio::sync::mpsc::unbounded_channel(); | |||
| let enclave_clone = enclave.clone(); | |||
| let mut rt = tokio::runtime::Builder::new() | |||
| .basic_scheduler() | |||
There was a problem hiding this comment.
As I understand basic_scheduler means everything runs on a single thread.
It's what_ we are aiming for.
But do we want to see if a (multi) threaded_scheduler improves performance?
There was a problem hiding this comment.
We can experiment with that, but I don't expect that would help. This might actually explain why we saw lower performance for tokio 0.2 implementation over 0.1 before, as we were using multi-threaded scheduler during that test for 0.2.
|
bors r+ |
Build succeeded
|
This is mostly the same as PR #210.
cc @Goirad