search

paritytech / parity-tokio-ipc

Parity tokio-ipc
Apache License 2.0
76 stars 48 forks source link

Migrate from `tokio_core` to `tokio`. #9

Closed c0gent closed 6 years ago

c0gent commented 6 years ago

I will change the dependency path back to the original location once https://github.com/NikVolf/tokio-named-pipes/pull/1 is merged.

NikVolf commented 6 years ago

added appveyour here as well, please rebase, sorry for CI issues!

c0gent commented 6 years ago

No problem :)

NikVolf commented 6 years ago

appveyor failed ^^

c0gent commented 6 years ago

Hmm, I'll have a look...

c0gent commented 6 years ago

Oh... recent changes added new windows stuff... I'll take care of it.

c0gent commented 6 years ago

Restarted CI check.

Am I correct in saying that the tests on Windows spuriously fail and/or hang? They have done so for me even before I made any changes.

NikVolf commented 6 years ago

Well no, last time I checked, now I don't have windows

NikVolf commented 6 years ago

You see, this build is ok:

https://ci.appveyor.com/project/NikolayVolf/parity-tokio-ipc/build/1.0.3

I'll run it few times to see if there are non-deterministic failures

NikVolf commented 6 years ago

So far every b4a6cdf7 commit build is a success... https://ci.appveyor.com/project/NikolayVolf/parity-tokio-ipc/history

c0gent commented 6 years ago

Interesting... I'll see what I can figure out.

c0gent commented 6 years ago

I've temporarily added a timeout to ipc pipe creation. Let me know if I should leave it in or not.

~The intermittent stalling / failures must be a result of having multiple threads attempting to access the Windows file handles at the same time (now that everything runs on a multi-threaded runtime). I think what I'm going to have to do is add back the handle argument to tokio_named_pipes::NamedPipe::new, ::from_pipe, etc. and also to parity_tokio_ipc::IpcConnection::connect. That way, a single reactor handle can be used to create them in a single-threaded fashion once again.~

I'm not sure what the problem is yet. I don't see how anything I've changed would affect named pipe creation.

c0gent commented 6 years ago

Using a reactor handle for NamedPipe creation did the trick. Apparently this is important for coordination with tokio::reactor::PollEvented2 on Windows due to the single-threadyness needed.

NikVolf commented 6 years ago

cool, just a couple of minor issues

c0gent commented 6 years ago

Will update dependency as soon as https://github.com/NikVolf/tokio-named-pipes/pull/2 is merged.

c0gent commented 6 years ago

Also instead of creating a stable branch, I'd create a separate branch for each version. Or use tags instead of branches.

NikVolf commented 6 years ago

@c0gent we'll leave this version management to cargo once it will be published on crates.io

c0gent commented 6 years ago

@NikVolf I assume you're fine with taking over the tokio-named-pipes crate, yeah? I'll help maintain if needed.

NikVolf commented 6 years ago

Aye, and I don’t see any problems doing it in current gh state