search

locka99 / opcua

A client and server implementation of the OPC UA specification written in Rust
Mozilla Public License 2.0
499 stars 131 forks source link

Upgrading actix-web #291

Open antiblue opened 1 year ago

antiblue commented 1 year ago

Hi all, I started working on upgrading the Actix-Web parts, but I stumble over an issue with Tokio.

Through OPCUASession.connect the function get_server_endpoints_from_url<T> is called and at the end an instance of Session is dropped. This causes a panic, because Tokio wants to enter a blocking region:

thread 'actix-rt|system:0|arbiter:3' panicked at 'Cannot drop a runtime in a context where blocking is not allowed. This happens when a runtime is dropped from within an asynchronous context.', ...tokio-1.32.0\src\runtime\blocking\shutdown.rs:51:21

I have found running-actix-web-using-tokiomain in Actix-Web's documentation, stating that block_in_place will not work. Unfortunately the backtrace reveals exactly that:

  13: core::ptr::drop_in_place<opcua::client::session::session::Session>
             at /rustc/5680fa18feaa87f3ff04063800aec256c3d4b4be\library\core\src\ptr\mod.rs:497
  14: opcua::client::client::Client::get_server_endpoints_from_url<ref$<str$> >
             at ...\opcua\lib\src\client\client.rs:493

Any suggestions on how to fix this?

(The code has been forked here)

AiyionPrime commented 3 months ago

@antiblue I'm struggling with the same problem. Was just drafting a different example structure in #355, when I hit this error as well. Did you make any progress with this? I'd really like to see the example having the same actix version as the rest of the code.

antiblue commented 3 months ago

No, sorry.

AiyionPrime commented 3 months ago

After reading through the other PRs and recent commits, maybe @einarmo might be a good choice to ask for directions about this problem.

einarmo commented 3 months ago

You shouldn't be having a trouble with a runtime being dropped in the client now, since the client no longer contains a runtime. Is this hitting somewhere else?

AiyionPrime commented 3 months ago

I encountered the problem when I drafted an update of samples/web-client to the actix version in the lib crate. I'll provide a PR tomorrow; today's kind of full; sorry.

vlnzrv commented 2 months ago

Hi @AiyionPrime! Any updates about this PR with the actix version upgrade?

AiyionPrime commented 2 months ago

Hey @vlnzrv, sorry the topic has spread over various PRs and issues now.

I talked to the maintainer @locka99 a week ago, where he made clear that he was going to look into how to enable external support of this library. But he also remarked his currently quite limited time, of which he can't spend as much in this project.

That and the amount of open PRs (about 18) have led me to stop opening PRs for the moment, until we find a way to reduce (and hopefully merge some) again.

My goal is to help this project, not drown it in work. Nevertheless that's what I've already told @einarmo in a different context and was not aware others were awaiting this as well.

vlnzrv commented 2 months ago

Hey @AiyionPrime, thanks for the update and your contributions! Some comments why I was looking for a way to update it. In May rust nightly broke for old versions of time crate: https://github.com/rust-lang/rust/issues/125319. And it's not going to be fixed according to that thread, so the only way around is to upgrade time crate. Opcua crate depends on time crate via actix-web, so it's a blocker for an update. In a couple of weeks rust stable will stop working for these old versions of time crate, so It'll be an annoying issue.

AiyionPrime commented 2 months ago

@vlnzrv Yeah I saw that, too. It was not meant as "won't do" :) I opened a draft PR #360 to discuss the shortcomings of my implementation.

One definitely is the non graceful shutdown during ctrl+c which results in the runtime getting terminated in async context. I think I have an idea for that though.

My code is not tested at all; I've got an interview to setup now, but will get back to this later in the evening. Hope this helps anyway.

AiyionPrime commented 2 months ago

@vlnzrv 1.80.0 is happening next Thursday, isn't it?

AiyionPrime commented 2 months ago

Furthermore it's only the sample which is running the ancient version of time, if I'm not mistaken? So temporarily dropping it would be a bad, but available fallback, I think.

vlnzrv commented 2 months ago

Yes, it's only a sample, because dependency in the lib itself was updated. In the worst case the sample could be just commented in Cargo.toml until it's fixed. It won't break unit / integration tests.

vlnzrv commented 2 months ago

.80.0 is happening next Thursday, isn't it?

Correct: https://releases.rs/docs/1.80.0/

AiyionPrime commented 2 months ago

Alright. The PR I drafted to address the beta issue is #361. And the draft to verify the CI addition works is #362.

@vlnzrv Furthermore for your downstream repo you should be fine performing the same addition to the Cargo.toml in case this does not get merged in time.