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

Bugfix: Close session properly if activation failed #657

Merged

Conversation

Maddin-619
Copy link
Contributor

At the moment if the session activation failed the session dosn't get closed. Depending on the server session timeout there can accumulate lots of danggling sessions.

To close the session after a activation failure the session must be set before calling the Close function.

@magiconair
Copy link
Member

Thank you! I've refactored the change to make closing the session explicit and not just a side effect of setting the session. However, we still call setSession in the ActivateSession handler and there is a side-effect here. Question is at which point should the session variable contain a valid value. After creation or after activation? I'll merge it and release a v0.3.16 shortly together with your other fix.

@magiconair magiconair added this to the v0.3.16 milestone Jun 5, 2023
@magiconair magiconair added the bug Something isn't working label Jun 5, 2023
@magiconair magiconair merged commit cc2711e into gopcua:main Jun 5, 2023
@magiconair magiconair modified the milestones: v0.3.16, v0.4.0 Jun 13, 2023
@magiconair
Copy link
Member

This will go into v0.4.0

@Maddin-619
Copy link
Contributor Author

@magiconair In the current version the bug occurs again. A good PR should have include some tests, damn it. Sorry about that.
It's basically the same problem as the last time: If ActivateSession fails, the session is not stored in the atomic variable and therefore cannot be closed. Are you good if i submit another PR? If yes do you prefer the explicit way again?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants