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

Easy to misuse API for closing client #689

Open
lbjarre opened this issue Sep 5, 2023 · 0 comments
Open

Easy to misuse API for closing client #689

lbjarre opened this issue Sep 5, 2023 · 0 comments

Comments

@lbjarre
Copy link

lbjarre commented Sep 5, 2023

v0.5.0 of gopcua changed the signature of (*Client).Close to accept a context.Context for carrying cancellation signals. However, this means that the "obvious" way of using the client will now be prone to bugs:

c := opcua.NewClient(endpoint)
if err := c.Connect(ctx); err != nil {
    return err
}
defer c.Close(ctx)

If we use the same cancellation signals for the (*Client).Close call and the parent context is cancelled, we will just skip any cleanup and terminate the connection without correctly releasing resources on the server.

This is of course resolvable in user code by separating the contexts used, but I think that it still warrants some discussion about the API. I see three possible ways of implementing the method:

  1. Take context.Context, respect cancellation signal. This is the current behavior, and while it is the most predicable option it is as mentioned prone to misuse. This should probably be pointed out in documentation.
  2. Take context.Context, ignore cancellation signal. This would make the obvious usage more correct, at the cost of making the code less predictable. It does seem weird to completely ignore the input given by the user, and would probably lead to more subtle bugs down the line.
  3. Don't take context.Context, use internal cancellation signals. Would be difficult to misuse, but would also take away control from the user.

I think that I'm still in favor of the current behavior, just that it needs to be called out more explicitly in documentation IMO.

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

No branches or pull requests

1 participant