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

Add SetReadDeadline for Conn #649

Open
cckolya opened this issue May 4, 2023 · 13 comments
Open

Add SetReadDeadline for Conn #649

cckolya opened this issue May 4, 2023 · 13 comments

Comments

@cckolya
Copy link

cckolya commented May 4, 2023

Hi!

if _, err := io.ReadFull(c, b[:hdrlen]); err != nil {

if _, err := io.ReadFull(c, b[hdrlen:h.MessageSize]); err != nil {

This code snippet blocks a running program while network is down. I suggest adding something like this before io.ReadFull()

c.SetReadDeadline(time.Now().Add(time.Second * c.readDeadline))

This will solve the following problems:

  1. After the network is disconnected for about 20 seconds, the program is blocked for 3 minutes (this is the default value of net. Conn timeout)
  2. The application understands after 3 minutes when the OPC server is down or the connection between the application and the opc server is broken

How do I reproduce my problems?

  1. Subscribe to one or more OPC server tags.
  2. Just turn off the network for about 20 seconds

Example how I create opcua.Client:

opts := make([]opcua.Option, 0)
opts = append(opts, opcua.RequestTimeout(time.Second*5))
opts = append(opts, opcua.DialTimeout(time.Second*5))

opcua.NewClient(settings.Url, opts...)
@cckolya cckolya changed the title Add SetReadDeadline for net.Conn Add SetReadDeadline for Conn May 4, 2023
@kung-foo
Copy link
Member

Seems reasonable. Can you make a PR?

@kung-foo
Copy link
Member

Though this would need to be deconflicted with the calls to SetDeadline that are based on the incoming context.

@magiconair
Copy link
Member

@kung-foo what would a cleaner solution look like? We can break the API with v0.5 if it makes sense since we're going to anyway.

@magiconair
Copy link
Member

Also, would all calls require the same ReadDeadline? What about a PublishRequest? That will wait for a long time

@kung-foo
Copy link
Member

Yeah, was thinking later about some of the other pub/sub style calls. I'll need to read more into how keep-alives are implemented (i.e. TCP vs OPC-UA keep alive message). Just a few weeks ago we ran into an issue where Aveva would do SYN,SYNACK,ACK and then nothing. Using #629 "fixed" it.

@kung-foo
Copy link
Member

At minimum, every call that takes a context should apply a deadline (if the context has one).

@cckolya
Copy link
Author

cckolya commented May 15, 2023

I found this PR #628 will fix this issue

@cckolya cckolya closed this as completed May 31, 2023
@magiconair magiconair reopened this Jun 13, 2023
@magiconair
Copy link
Member

@Kolyan4ik99 can you test what the read deadline does with PublishRequests ?

@magiconair
Copy link
Member

I'm a bit reluctant to merge #628 without knowing that.

@cckolya
Copy link
Author

cckolya commented Jun 13, 2023

If code
c. readTimeout == 1 or c. readTimeout == time.Now().Add(1) for example, then
n, err := c.Read(b[:hdrlen]) instantly return err that time is expired

I am using this change in a project and it creates some new problems. My OPC server updates parameters by subscription once every 2 minutes, and I set readTimeout = time.Now().Add(time.Minute):

  • If the OPC server does not return the parameters on the subscription until the readTimeout has expired, then the subscription is closed and I must re-subscribe. Or I must set readTimeout = the interval of the most frequently updated parameter

I need to know the interval of the most frequently updated parameter or use a re-subcribiction

There were no other problems

@magiconair
Copy link
Member

magiconair commented Jun 14, 2023

So this means that the read timeout depends on the type of request? For anything but the PublishRequest we can use the configured value but for PublishRequest we don't set it at all or make it dependent on the subscriptions? Note that the OPC server only sends updates if values actually change. There is no guarantee that you'll get a response AFAIU. Ignore this because it is irrelevant.

The uacp.Conn#Receive method reads the next message from the stream. If the server does not send any message within the ReadDeadline time limit we get an os.ErrDeadlineExceeded error. The question is how to interpret this.

The secure channel dispatcher runs in the background always receiving messages from the server. It does not and cannot know if and when the server sends a message since there are no periodic keep alive messages coming from the server.

It is valid for the OPC server to not send data. I can just open a secure channel and not do anything with it. Or monitor a value that never changes.

So setting a ReadDeadline applies to all messages and means that the user expects the server to always send a message at least every so often. Otherwise, we get an error. I think the only sensible thing to do is to close the connection.

@magiconair
Copy link
Member

context deadlines are specific to a request and shouldn't necessarily close the connection or am I off-base here?

@cckolya
Copy link
Author

cckolya commented Jul 11, 2023

Context terms don't close the connection, it's local logic
If we send/write something from conn and the deadline has passed, we will just get an error and have to determine what we want
We can increase the deadline value for the second send/write operation and close the connection after the third attempt, for example.
https://pkg.go.dev/net#Conn

Yes, ReadDeadline applies to all messages for the conn instance, and after reading it is more correct to set the value to SetReadDeadline(time.Time{}). This correct for WriteDeadline too

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

3 participants