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

Fix always schedule renewal of security token #466

Merged
merged 1 commit into from
Sep 23, 2021

Conversation

vereecw
Copy link
Contributor

@vereecw vereecw commented Sep 23, 2021

Fix for issue #392

Link to reason in OPC-UA documentation in comment in the issue.

We've also taken a look at the C library for OPC-UA and found no check for MessageSecurityMode.

renewSecureChannel
function call 1
function call 2

@magiconair
Copy link
Member

Hi @VereeckenWannes, I assume you've tested that on your setup. @kung-foo added that in #370 Maybe there was a specific reason.

@magiconair
Copy link
Member

Lets merge this and see if this breaks things.

@magiconair magiconair merged commit f860ce1 into gopcua:main Sep 23, 2021
@magiconair magiconair added this to the v0.2 milestone Sep 23, 2021
@kung-foo
Copy link
Member

@magiconair honestly, 99% of my testing is against "secure" servers. I never did any lifetime/session testing against servers with a security mode of none.

IIRC I did have a patched version of a node-opcua-based server where I forced the session lifetime to something extremely short (i.e. 15 seconds). Maybe it's worth resurrecting something like that for the integration tests.

@vereecw
Copy link
Contributor Author

vereecw commented Sep 24, 2021

@magiconair yes, we tested in on multiple setups and this change fixed our issue with losing our opcua connection after the lifetime expires when SecurityMode is set to none.

@magiconair
Copy link
Member

@kung-foo that sounds like a good idea. @VereeckenWannes thank you.

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