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

ipc: Allow specifying different event loop reactor in server builder #459

Merged
merged 2 commits into from
Jul 24, 2019

Conversation

Xanewok
Copy link
Contributor

@Xanewok Xanewok commented Jul 20, 2019

Right now we use Handle::default() (which should lazily bind to a reactor) when creating an IpcConnection but that surprisingly doesn't work with either the user-specified executor or the lazily spawned pool as the fallback executor on Windows (only tested on Windows 10).

Using a simple Runtime::reactor works, so to work around the issue I'd like be able to specify a custom reactor to be used (which seems somewhat useful on its own, hopefully).

Disclaimer: I'm not yet well versed in the async-fu so I might be missing something obvious here, please correct me if I'm wrong.

@parity-cla-bot
Copy link

It looks like @Xanewok hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io

Once you've signed, please reply to this thread with [clabot:check] to prove it.

Many thanks,

Parity Technologies CLA Bot

@Xanewok
Copy link
Contributor Author

Xanewok commented Jul 20, 2019

[clabot:check]

@parity-cla-bot
Copy link

It looks like @Xanewok signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

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.

Yeah, the way we integrate between tokio and tokio-core and old-async is pretty messy now, see #425 .
If this solves some issues for you, let's merge it.

@tomusdrw tomusdrw requested a review from dvc94ch July 22, 2019 09:10
@Xanewok
Copy link
Contributor Author

Xanewok commented Jul 22, 2019

Yeah, it seems that named pipes require a consistent thread for the wakeups (cc NikVolf/tokio-named-pipes#2 and @c0gent who authored the change) so it seems necessary to be able to provide a user-specified handle.

Not sure if this changes semantics as we now construct Handle::default() outside the lazily spawned future now (tokio-rs/tokio#307)

The difference is that Handle::default will use the global reactor if called from outside of a runtime. This global reactor will most likely go away in 0.2 and was from pre Runtime.

I can use Option<Handle> for ServerBuilder::reactor and then inside the future use

let reactor = self.reactor.unwrap_or_else(Handle::default);

to bring back the original behaviour in case user didn't override the previous default reactor.

@tomusdrw
Copy link
Contributor

can use Option for ServerBuilder::reactor and then inside the future use..

@Xanewok yeah, that makes sense to me

@Xanewok
Copy link
Contributor Author

Xanewok commented Jul 22, 2019

Done, added a comment why we need to create a default value inside a future

@Xanewok
Copy link
Contributor Author

Xanewok commented Jul 22, 2019

test transports::http::tests::handles_connection_refused_error ...
test transports::http::tests::handles_connection_refused_error has been running for over 60 seconds

that's unfortunate (also happened in #460 (comment))

@Xanewok Xanewok closed this Jul 22, 2019
@Xanewok Xanewok reopened this Jul 22, 2019
@tomusdrw tomusdrw merged commit 9cb109c into paritytech:master Jul 24, 2019
@Xanewok Xanewok deleted the event-loop-reactor branch July 24, 2019 12:53
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.

3 participants