Batching writes

[tomkcook]

Apologies if this is my ignorance showing through. Is there some way to batch up writes to the OPC UA server? Called Node::SetValue seems to send one packet to the server per write, AFAICT. Is there some way to send many writes in one server operation?

[oroulet]

Set_values takes an array of AttributeValue as arg and is to supposed to send one ua write call,thus executing many write operations in one call. Doesn't it work?

On Thu, Jun 25, 2015, 15:50 tomkcook notifications@github.com wrote:

Apologies if this is my ignorance showing through. Is there some way to batch up writes to the OPC UA server? Called Node::SetValue seems to send one packet to the server per write, AFAICT. Is there some way to send many writes in one server operation?

— Reply to this email directly or view it on GitHub https://github.com/FreeOpcUa/freeopcua/issues/156.

[tomkcook]

Far from working, it doesn't even seem to be there. I've grepped header and source files for a wide variety of case-insensitive versions of 'Set_', '_values', 'update, 'values', 'attributevalue', 'attributemap' and so one, with nothing that appears related to 'Set_values'. There is no method that takes an array of AttributeValue (or even a single instance of AttributeValue).

[oroulet]

Looks like I answered without checking here. it seems it is not implemented. It is really not hard to implement though, just look at the code of Node::SetValue The 'hard' part is to decide where this method/function should be defined and what it should take as argument... I let you have a look

On Fri, 26 Jun 2015 at 12:19 tomkcook notifications@github.com wrote:

Far from working, it doesn't even seem to be there. I've grepped header and source files for a wide variety of case-insensitive versions of 'Set_', '_values', 'update, 'values', 'attributevalue', 'attributemap' and so one, with nothing that appears related to 'Set_values'. There is no method that takes an array of AttributeValue (or even a single instance of AttributeValue).

— Reply to this email directly or view it on GitHub https://github.com/FreeOpcUa/freeopcua/issues/156#issuecomment-115630099 .

[tomkcook]

Gladly. I'm glad it wasn't just me being daft!

I'd guess that it should be a method on the UaClient class - it doesn't really make much sense to make it a member function on Node. A static member of Node might make some sense, but then you'd have to pass in all sorts of extra stuff to make it work.

[oroulet]

I would also like to have it as member of UaServer. so maybe create a function SetAttribeValues under OpcUa taking a Server as parameter and add the method both on UaClient and UaServer.....

On Fri, 26 Jun 2015 at 12:49 tomkcook notifications@github.com wrote:

Gladly. I'm glad it wasn't just me being daft!

I'd guess that it should be a method on the UaClient class - it doesn't really make much sense to make it a member function on Node. A static member of Node might make some sense, but then you'd have to pass in all sorts of extra stuff to make it work.

— Reply to this email directly or view it on GitHub https://github.com/FreeOpcUa/freeopcua/issues/156#issuecomment-115642245 .

[tomkcook]

How do you feel about:

class UaUpdate {
    ...
public:
    template<typename T>
    void operator()(const Node& node, const T& value);

    void Execute();
};

class UaClient {
    ....
public:
    UaUpdate CreateUpdate();
};

Usage would be like this:

UaClient client = ...;
Node n1 = ...;
Node n2 = ...;

UaUpdate update = client.CreateUpdate();
update(n1, 1.0f);
update(n2, int16_t(15));
update.Execute();

As suggested, I'd make this work on both server and client.

[oroulet]

I had a short look at the code again, and I am wondering if we need such a helper class/function. Maybe it is better to expose more clearly Services::SharedPtr object both in UaClient and UaServer The underlying ua API is really not hard.

WriteValue att1; att1.NodeId = Id; att1.AttributeId = attr; att1.Value = dval; ... ... std::vector codes = client.GetServer()->Attributes()->Write(std::vector{att1, att2, ...});

In freeopcua we have a very high level interface with Node, UaClient, UaServer and Subcsriptions objects, but we should make it easy for users to access the underlying ua api to do more complex/efficient things. In this case we only need to add GetServer() to UaClient and UaServer

[tomkcook]

Well, then, how about exposing a new method on UaClient and UaServer, WriteAttributes(std::vector<WriteValue>)? Exposing the whole server object just to expose one method seems a bit overkill.

[oroulet]

just to expose one method seems a bit overkill. It is not only one method, there will be people wanting to read several nodes at once, doing special queries using the browse command, etc so yes I think we should use expose the server object in UaClient and Server, in Node object too so people can send custom requests,

On Mon, 29 Jun 2015 at 11:55 tomkcook notifications@github.com wrote:

Well, then, how about exposing a new method on UaClient and UaServer, WriteAttributes(std::vector)? Exposing the whole server object just to expose one method seems a bit overkill.

— Reply to this email directly or view it on GitHub https://github.com/FreeOpcUa/freeopcua/issues/156#issuecomment-116588992 .

[oroulet]

btw AddVariable AddMethod, etc methods are allready splitted from Node in the python implementation. Maybe we should add a class 'Helpers' for example that contain all these helper methods such as CreateVariable, CreateMethod, even WriteAttributes if you want.

On Mon, 29 Jun 2015 at 12:38 Olivier Roulet-Dubonnet < olivier.roulet@gmail.com> wrote:

just to expose one method seems a bit overkill. It is not only one method, there will be people wanting to read several nodes at once, doing special queries using the browse command, etc so yes I think we should use expose the server object in UaClient and Server, in Node object too so people can send custom requests,

On Mon, 29 Jun 2015 at 11:55 tomkcook notifications@github.com wrote:

Well, then, how about exposing a new method on UaClient and UaServer, WriteAttributes(std::vector)? Exposing the whole server object just to expose one method seems a bit overkill.

— Reply to this email directly or view it on GitHub https://github.com/FreeOpcUa/freeopcua/issues/156#issuecomment-116588992 .

[tomkcook]

Mmmm, not sure I follow your thought, there (not familiar with the Python implementation). Can you flesh the idea out a bit? Perhaps I need more coffee this morning.

[oroulet]

AddVariable, AddObjects do not really belong to a Node object either. they were place there to allow chaining but we should move implementation to an external 'Helper' class (feel free to find a better name) together with your writeAttributes stuff We would have something like this

class UaClient { .... public: XXServer GetServer();

Helpers GetHelpers();

};

class Helpers {

public: Helpers(Server);

CreateObjectObject(const NodeId& folderId, const QualifiedName& browseName) const;

CreateVariable ...etc

WriteAttributes (If you want it)

}

class Node{

public:

AddObject(xxx) {Helper::CreateObject(Server, xxxx);}

.-...etc...

}

I am not sure this need to be a class either, we can use functions that take the Server object as first argument.

And feel free to propose a better name than 'Helpers'!

On Mon, 29 Jun 2015 at 12:52 tomkcook notifications@github.com wrote:

Mmmm, not sure I follow your thought, there (not familiar with the Python implementation). Can you flesh the idea out a bit? Perhaps I need more coffee this morning.

— Reply to this email directly or view it on GitHub https://github.com/FreeOpcUa/freeopcua/issues/156#issuecomment-116604404 .

[tomkcook]

I've been having a crack at this today, but have run into another, somewhat more serious, problem.

The object BinaryClient::Stream appears to be shared between threads without any protection. At the very least:

• binary_client.cpp:212 sets up a thread which calls Receive() repeatedly. This uses the Stream object to deserialise data from the server.
• BinaryClient::Write (binary_client.cpp:318) calls Send<Response, Request>(Request), which calls Send<Request>(Request), which uses the Stream object to serialise data to be sent to the server.

In a situation where the client is subscribed to events on the server, and so packets are arriving asynchronously, and writes to the server are also being made, this results in conflicting use of the Stream object, leading to data corruption, debug assertion failures and related problems.

I imagine that reproducing the problem reliably will depend somewhat on the server being used. As before, I'm testing this against a B&R X20 PLC. I can fairly reliably reproduce problems with the following code, inserted near the end of main in example_client.cpp:

    //Subscription
    std::vector<std::string> write_varpath{ ... path to a writable node ... };
    myvar = objects.GetChild(write_varpath);

    SubClient sclt;
    std::unique_ptr<Subscription> sub = client.CreateSubscription(100, sclt);
    uint32_t handle = sub->SubscribeDataChange(myvar);
    std::cout << "Got sub handle: " << handle << ", sleeping 5 seconds" << std::endl;
    std::this_thread::sleep_for(std::chrono::seconds(5));

    std::vector<WriteValue> update;
    for (int ii = 0; ii < 1000; ii++) {
        update.clear();
        WriteValue val;
        val.NodeId = myvar.GetId();
        val.AttributeId = AttributeId::Value;
        val.Value = DataValue(Variant(float(ii) / 1000.0f));
        update.push_back(val);
        client.GetServer()->Attributes()->Write(update);
        std::cout << '.';
    }
    std::cout << "\nDisconnecting" << std::endl;
    client.Disconnect();
    return 0;
}

I've run this about ten times and twice it has produced output like this:

Connecting to: opc.tcp://10.59.121.56:4840/
Connection was closed by host. No error.
Root node is: Node(ns=0;i=84;)
Child of objects node are:
    Node(ns=0;i=2253;): Server
    Node(ns=2;i=5001;): DeviceSet
    Node(ns=4;i=20000;): PLC
NamespaceArray is:
    http://opcfoundation.org/UA/
    urn:br-automation/BuR/UA/EmbeddedServer
    http://opcfoundation.org/UA/DI/
    http://PLCopen.org/OpcUa/IEC61131-3/
    urn:B&R/plc/

    urn:B&R/pv/
    urn:PLCopen/pv/
Got sub handle: 1, sleeping 5 seconds
Received DataChange event, value of Node Node(ns=6;s=::WTC:plc_CI_AlgPitPos1;) is now: 0.999
....................Received DataChange event, value of Node .Node(ns=6;s=::WTC:plc_CI_AlgPitPos1;). is now: 0.003
..................................................Received DataChange event, value of Node .Node(ns=6;s=::WTC:plc_CI_AlgPitPos1;). is now: 0.055
................................................................Received DataChange event, value of Node .Node(ns=6;s=::WTC:plc_CI_AlgPitPos1;) is now: 0.12.
.binary_client| No callback found for message with id: ns=0;i=676; and handle 160
.............................................................Received DataChange event, value of Node .Node(ns=6;s=::WTC:plc_CI_AlgPitPos1;). is now: 0.178
................................................................Received DataChange event, value of Node .Node(ns=6;s=::WTC:plc_CI_AlgPitPos1;). is now: 0.25
..................................................................Received DataChange event, value of Node .Node(ns=6;s=::WTC:plc_CI_AlgPitPos1;). is now: 0.313
.................................................................Received DataChange event, value of Node .Node(ns=6;s=::WTC:plc_CI_AlgPitPos1;) is now: 0.38
.............................................................Received DataChange event, value of Node .Node(ns=6;s=::WTC:plc_CI_AlgPitPos1;) is now: 0.447.
Connection was closed by host. No error.
Error: received empty packet from server
.Error: received empty packet from server
.Error: received empty packet from server
.Error: received empty packet from server

[tomkcook]

Note that the example code posted above depends on a change adding the GetServer() method to UaClient.

[tomkcook]

Any ideas on how best to fix this are very welcome! I've tried constructing a second Stream object as a local variable in Receive(), but it doesn't seem to help. Does the IOChannel suffer from the same problem? Or have I completely mis-diagnosed the problem?

[oroulet]

I am not the author og that part of the code. But I remember discussing this with the author and he was 100% sure that his code was threadsafe. I also tried to run valgrind on it that could not see much. But I am not a valgrind expert.

Try adding a huge global lock and tell us if it helps. If it does then we can make up a better solution

On Mon, Jun 29, 2015, 18:22 tomkcook notifications@github.com wrote:

Any ideas on how best to fix this are very welcome! I've tried constructing a second Stream object as a local variable in Receive(), but it doesn't seem to help. Does the IOChannel suffer from the same problem? Or have I completely mis-diagnosed the problem?

— Reply to this email directly or view it on GitHub https://github.com/FreeOpcUa/freeopcua/issues/156#issuecomment-116748938 .

[oroulet]

I had a look again and I am also wondering if there is an issue here. we can put a lock SocketChannel class. but there may also be some buffer issues. Maybe putting a lock in stream.h and see if it helps

On Mon, 29 Jun 2015 at 19:38 Olivier Roulet-Dubonnet < olivier.roulet@gmail.com> wrote:

I am not the author og that part of the code. But I remember discussing this with the author and he was 100% sure that his code was threadsafe. I also tried to run valgrind on it that could not see much. But I am not a valgrind expert.

Try adding a huge global lock and tell us if it helps. If it does then we can make up a better solution

On Mon, Jun 29, 2015, 18:22 tomkcook notifications@github.com wrote:

Any ideas on how best to fix this are very welcome! I've tried constructing a second Stream object as a local variable in Receive(), but it doesn't seem to help. Does the IOChannel suffer from the same problem? Or have I completely mis-diagnosed the problem?

— Reply to this email directly or view it on GitHub https://github.com/FreeOpcUa/freeopcua/issues/156#issuecomment-116748938 .

[oroulet]

looking again I do not see any buffer anywhere. so putting a lock in SocketChannel should be enough. Do you see something else?

On Mon, 29 Jun 2015 at 21:21 Olivier Roulet-Dubonnet < olivier.roulet@gmail.com> wrote:

I had a look again and I am also wondering if there is an issue here. we can put a lock SocketChannel class. but there may also be some buffer issues. Maybe putting a lock in stream.h and see if it helps

On Mon, 29 Jun 2015 at 19:38 Olivier Roulet-Dubonnet < olivier.roulet@gmail.com> wrote:

I am not the author og that part of the code. But I remember discussing this with the author and he was 100% sure that his code was threadsafe. I also tried to run valgrind on it that could not see much. But I am not a valgrind expert.

Try adding a huge global lock and tell us if it helps. If it does then we can make up a better solution

On Mon, Jun 29, 2015, 18:22 tomkcook notifications@github.com wrote:

Any ideas on how best to fix this are very welcome! I've tried constructing a second Stream object as a local variable in Receive(), but it doesn't seem to help. Does the IOChannel suffer from the same problem? Or have I completely mis-diagnosed the problem?

— Reply to this email directly or view it on GitHub https://github.com/FreeOpcUa/freeopcua/issues/156#issuecomment-116748938 .

[tomkcook]

Here's some more information on this.

The failures come in three categories:

• Sometimes there is a message printed, "...". After this, the thread doing writes takes ten seconds for every write, presumably because there is no response from the server. I'm guessing that this is the server hanging up because it receives a mal-formed packet (due to data corruption on the client side). This is the most common type of failure I'm seeing.
• Sometimes the subscription handler prints a message saying that it has received a subscription notification for an unknown handle. The handle number is always close to the number of attribute writes done (I don't know precisely as I'm not collecting statistics at that fine grain). This suggests that the content of one packet is being overwritten by the content of another while the first is still being processed. This is the least common type of failure. It is not fatal and is almost always followed by one of the other types a few cycles later.
• Sometimes a debug assertion fails, typically at binary_stream.cpp:721. I'm not clear on the exact mechanism here, but it seems that somehow std::vector<char>::begin() is returning an invalid iterator. This is less common than the first type but more common than the second type.

Having collected some statistics, it seems that BinaryClient::Publish is being called when I don't expect it to be, from a different thread to all the other operations, and that this is related to the failures. I've run tests, some with the code as at HEAD and others with BinaryClient::Publish stubbed out to do nothing.

With Publish still enabled, I've run ten tests similar to the sample code further up this thread. None has managed 100,000 attribute updates without displaying one of the problems above. The average is 15,200 and the sample standard deviation 26,000 (note that all numbers are floored to multiples of 1,000).

The same test running with Publish stubbed out has just complete 1,000,000 attribute writes without failure. This seems to me reasonably conclusive evidence that there is a threading vulnerability in Send. Modifying Send to look like this also seems to resolve the problem:

mutable std::mutex send_mutex;
template <typename Request>
void Send(Request request) const
{
    std::unique_lock<std::mutex> send_lock(mutex);
    // TODO add support for breaking message into multiple chunks
    SecureHeader hdr(MT_SECURE_MESSAGE, CHT_SINGLE, ChannelSecurityToken.SecureChannelId);
    ...

This has now completed over 250,000 attribute writes (with Publish not stubbed out) without failure.

[tomkcook]

Also, modifying Send to this also seems to fix the problem:

mutable std::mutex send_mutex;

template <typename Request>
void Send(Request request) const
{
  // TODO add support for breaking message into multiple chunks
  SecureHeader hdr(MT_SECURE_MESSAGE, CHT_SINGLE, ChannelSecurityToken.SecureChannelId);
  const SymmetricAlgorithmHeader algorithmHeader = CreateAlgorithmHeader();
  hdr.AddSize(RawSize(algorithmHeader));

  const SequenceHeader sequence = CreateSequenceHeader();
  hdr.AddSize(RawSize(sequence));
  hdr.AddSize(RawSize(request));

  std::unique_lock<std::mutex> send_lock(send_mutex);
  Stream << hdr << algorithmHeader << sequence << request << flush;
}

ie. have the lock just protecting the stream write operation.

Looking at it again, if Send is being called from two separate threads then it seems pretty obvious that this is a vulnerability. Each << in that line represents a separate function call, so even if the stream operations are thread-safe, they can still be interleaved, leading to a packet that has portions of two separate requests in it. It's hard, then, to see how the lock could be moved further down the call-tree.

[oroulet]

Thanks for your efforts debugging this. I am completely fine a lock there. If it solves your issue, send a pull request

On Tue, 30 Jun 2015 at 15:24 tomkcook notifications@github.com wrote:

Also, modifying Send to this also seems to fix the problem:

mutable std::mutex send_mutex;

template void Send(Request request) const { // TODO add support for breaking message into multiple chunks SecureHeader hdr(MT_SECURE_MESSAGE, CHT_SINGLE, ChannelSecurityToken.SecureChannelId); const SymmetricAlgorithmHeader algorithmHeader = CreateAlgorithmHeader(); hdr.AddSize(RawSize(algorithmHeader));

const SequenceHeader sequence = CreateSequenceHeader(); hdr.AddSize(RawSize(sequence)); hdr.AddSize(RawSize(request));

std::unique_lockstd::mutex send_lock(send_mutex); Stream << hdr << algorithmHeader << sequence << request << flush; }

ie. have the lock just protecting the stream write operation.

Looking at it again, if Send is being called from two separate threads then it seems pretty obvious that this is a vulnerability. Each << in that line represents a separate function call, so even if the stream operations are thread-safe, they can still be interleaved, leading to a packet that has portions of two separate requests in it. It's hard, then, to see how the lock could be moved further down the call-tree.

— Reply to this email directly or view it on GitHub https://github.com/FreeOpcUa/freeopcua/issues/156#issuecomment-117182603 .

[oroulet]

Thanks for your efforts debugging this. I am completely fine with a lock there. If it solves your issue, send a pull request. But if there is a bug here, there is probably the same issue on the server....

On Tue, 30 Jun 2015 at 15:34 Olivier Roulet-Dubonnet < olivier.roulet@gmail.com> wrote:

Thanks for your efforts debugging this. I am completely fine a lock there. If it solves your issue, send a pull request

On Tue, 30 Jun 2015 at 15:24 tomkcook notifications@github.com wrote:

Also, modifying Send to this also seems to fix the problem:

mutable std::mutex send_mutex;

template void Send(Request request) const { // TODO add support for breaking message into multiple chunks SecureHeader hdr(MT_SECURE_MESSAGE, CHT_SINGLE, ChannelSecurityToken.SecureChannelId); const SymmetricAlgorithmHeader algorithmHeader = CreateAlgorithmHeader(); hdr.AddSize(RawSize(algorithmHeader));

const SequenceHeader sequence = CreateSequenceHeader(); hdr.AddSize(RawSize(sequence)); hdr.AddSize(RawSize(request));

std::unique_lockstd::mutex send_lock(send_mutex); Stream << hdr << algorithmHeader << sequence << request << flush; }

ie. have the lock just protecting the stream write operation.

Looking at it again, if Send is being called from two separate threads then it seems pretty obvious that this is a vulnerability. Each << in that line represents a separate function call, so even if the stream operations are thread-safe, they can still be interleaved, leading to a packet that has portions of two separate requests in it. It's hard, then, to see how the lock could be moved further down the call-tree.

— Reply to this email directly or view it on GitHub https://github.com/FreeOpcUa/freeopcua/issues/156#issuecomment-117182603 .

[tomkcook]

Will do.

[tomkcook]

See #157.