Hi,

Attached is a patch which extents the jsonconf test suite. The new test highlights a rather big flaw in the workings of the TJSONConfig component. This issue might even apply to TXMLConfig component too - I haven't looked yet.

This came about in the discussion in the Lazarus mailing list about Lazarus IDE storing application settings in XML files. I investigated the idea of replacing my usage of INI files for storing application settings with JSON files (TJSONConfig) instead. In each of my applications, I store all GUI Form state (position, size, etc) all in one "application settings" file - each form has it's own XXXFormState section.

In the test TestConcurrency I simulate two forms being opened - each form contains a TJSONConfig component. When I close one form it writes the state information. When I close the second form, it overwrites the whole JSON file, wiping out the first form's information completely. This is rather dangerous behaviour!

I see 3 solutions here:

1) Use a Singleton with TJSONConfig so we only have one instance of TJSONConfig per application.
2) Extend TJSONConfig so that it is more intelligent when writing to a file, and only writes the path information it modified or created.
3) When using one TJSONConfig per form, force the developer to use a different .json file for each form.

Any comments or alternative solutions? I know XMLConfig and JSONConfig are not documented yet, but if nothing is going to be done about this concurrency issue I highlighted, then the documentation must clearly state the dangerous behaviour of these components.

Regards,
- Graeme -

Seriously, would you try the same thing with a file? Open it in two different places and try and write to it from both places and see what happens. Programs that want to write to the same json file from different classes need to handle this properly, just like they would have to when using files. The behaviour is completely logical, imho.

Henry

TXMLConfig reads by default the old config.
You can switch this off to start with a clean config. Unless you use the same paths, no value is lost.

Mattias

Op 2010-06-21 15:34, Henry Vermaak het geskryf:

Seriously, would you try the same thing with a file?

Seriously, you would want to tell developers in todays times that they can't concurrently access a file?

Programs that want to write to the same json file from different classes need to handle this properly, just like they would have to when using files.

Unfortunately TJSONConfig and TXMLConfig encapsulates the file handling - it's not available to the developer to change. So the file handling fix needs to occur in these components themselves.

Lazarus IDE even makes the TXMLConfig component available to developers on the System component palette - ready to be dropped on any form. All without the warning of potential data loss (even with StartEmpty = False)!

Regards,
- Graeme -

[...Sorry, I think my first message's attachment was too big. This one is smaller...]

Mattias Gaertner wrote

TXMLConfig reads by default the old config. Unless you use the same paths, no value is lost.

Op 2010-06-21 15:38, Mattias Gaertner het geskryf:

Unless you use the same paths, no value is lost.

Sorry Mattias, but you are wrong. TXMLConfig shows the exact same issue as TJSONConfig. Here is the output after the test run. I attached the xmlconfig test suite.

Test is simple:
- load form 1
- load form 2
- close form 1 > which saves settings
- close form 2 > which saves settings and overwrites everything.

---------------------------------------
$ ./testxmlconf --format=plain -a
Time:00.001 N:1 E:0 F:1 I:0
TTestXMLConfig Time:00.001 N:1 E:0 F:1 I:0
00.000 TestConcurrency Failed: Failed on 1 expected: <400> but was: <100>

Number of run tests: 1
Number of errors: 0
Number of failures: 1

List of failures:
Failure:
Message: TTestXMLConfig.TestConcurrency: Failed on 1 expected: <400> but was: <100>
Exception class: EAssertionFailedError
Exception message: Failed on 1 expected: <400> but was: <100>

$ cat concurrency.xml
<?xml version="1.0"?>
<CONFIG>
<LearnerListForm Width="456" Height="600" Maximized="False"/> </CONFIG>

---------------------------------------

No, that's not what I'm saying. You need to make it work. Your complaint is like saying the write syscall doesn't do the right thing when I use it on the same file from two different places. It _can_ work, but it's your job to do it correctly.

You can create a class that wraps all the json operations, and therefore handle concurrency in one place. You're only using one json file after all.

Henry

Now I understand.
Yes, TXMLConfig has the same "design flaw" as
TStringList.LoadFromFile/SaveToFile.
It is strange how you get used to such flaws and think it is normal.

Mattias

The same happens if you use .INI files. I don't see this as a problem.

The best way to avoid it is to create/destroy the config object after use. When the form is opened, you create an ini/json/whatever file object and read the data. Then destroy. When The form is closed, again create the file object (it should read the latest version of the file then) and then save changed values and destroy.

Likewise for a database application. Unless you take care of transaction support, 2 users editing a record will overwrite each other's changes, unless the programmer explicitly takes care of this.

In 12 years that our apps run, we used the system 'last writer wins' and no single user has complained yet about this.

Michael.

Op 2010-06-21 16:29, Henry Vermaak het geskryf:

You can create a class that wraps all the json operations, and therefore handle concurrency in one place.

Correct, that's one possible solution like I mentioned with the Singleton design pattern. But this still leaves the door wide open for others to have this problem because the actual TXXXConfig component doesn't do the "singleton/concurrency" by itself - and not many developers seem to know about this issue or know about the Singleton design pattern.

Regards,
- Graeme -

My point is that this is the wrong place to build such functionality in. How do you propose this should work from different files or processes?

Op 2010-06-21 16:31, Mattias Gaertner het geskryf:

Like I said, if nothing gets changed in the TXXXConfig components, then Lazarus IDE leaves the door wide open for developers to make this mistake. Why, because Lazarus IDE has a TXMLConfig component on the component palette. Developers wouldn't think twice that they are not allow to drop one component on each form and have a data loss issue.

Possible solutions:

* Fix TXXXConfig components to handle concurrency (multiple instances). More intelligent file handling.
* Implement a Singleton for each TXXXConfig component which is ready to use for by the developer. Just like Application and Screen are single instances in a application without the developer needing to do anything.
* Lazarus could maybe warn the developer (with a popup message) if they drop more than one TXXXConfig component in a project with same Filename value.
* Or TXMLConfig must be removed from Lazarus component palette as it's not like all other components that can handle multiple instances without problems.
* Or remember to add clear documentation (when it is written) about this pitfall with TXXXConfig components.

Regards,
- Graeme -

Op 2010-06-21 16:33, Michael Van Canneyt het geskryf:

The best way to avoid it is to create/destroy the config object after use. When the form is opened, you create an ini/json/whatever file object and

So then it's not wise to have TXMLConfig in the Lazarus component palette. Because dropping a component on a form doesn't give the behaviour you described.

Likewise for a database application. Unless you take care of transaction support, 2 users editing a record will overwrite each other's changes, unless the programmer explicitly takes care of this.

Doesn't Delphi's TClientDataset popup a 'conflict resolution dialog' in such a cases. I remember seeing such dialogs years ago (when I still used Delphi).

In 12 years that our apps run, we used the system 'last writer wins' and no single user has complained yet about this.

Because they don't get notified about their data (last saved) being lost. :-)

Michael, how does documentation work in FCL? Can we start documentation on these units and include these usage warnings - then not let them appear in official documentation until the FCL-json or FCL-xml packages are 100% documented? I know FPC has the policy that it doesn't allow "partial" documentation. I just fear that not documenting it now (somewhere) - this information will be forgotten when the docs are eventually written.

Regards,
- Graeme -

It does if you set 'active' or 'enabled' to false, no ?

Only if you program it so. I program with UpdateMode=upWhereKeyOnly, and then all updates go through, unless the record was deleted.

Well, I prefer that over bugging our users with dialogs that they don't understand anyway. Additionally, it drastically degrades performance.

You can start documenting them if you wish. But they won't be included in the official docs till fully finished. (Actually, I started on fcl-json to get it included on www.json.org. Documentation is a prerequisite)

Michael.

Op 2010-06-21 17:11, Michael Van Canneyt het geskryf:

[getting a bit off-topic here]

That is a rather dangerous approach to take. Imagine two accountants have a Clients transaction screen open - both adding new transactions. First accountant processes a 1 million Euro purchase. Then the second accountant processes a 50 Euro purchase. Using your thinking, that Client will now only owe use 50 euros. :-(

We handle concurrency slightly different. We use System Locks. The first person in a area get the system lock. Anybody else afterwards only sees that area as read-only with a option to send a message to the first user to get out of that area if it is urgent.

You can start documenting them if you wish. But they won't be included in the official docs till fully finished.

That's what I asked, and I'm fine with that. :-)

(Actually, I started on fcl-json to get it included on www.json.org. Documentation is a prerequisite)

I saw the message you forwarded from json.org - a reasonable prerequisite. I'm looking a bit more into JSON - it's a rather nice format. Easy to read (human readable) and differentiates nicely between arrays, integer, object and string types (describing data types) - much better than XML does.

eg:
<person name="john" age="28">
</person>

vs

{ "person": {
"name": "john",
"age": 28
}
}

The JSON output immediately tells me that name is a text field and age is an integer field. It's not obvious in the XML where all attribute values must be enclosed in quotes.

Regards,
- Graeme -

This only happens if developers thinks about TXMLConfig as a concurrent database component. This depends on the background and expectations of the developer. Any component can be misunderstood. As I started with Delphi I thought TList is a design flaw, because I started studying computer science and thought the term "list" in programs stands for linked lists and not for arrays. I'm sure that there are developers whos first impression about TXMLConfig would be "configurator", which is also wrong.

IMO TXMLConfig is already useful as it is. If someone needs concurrency and file locking it should be a descendant or a new class.

Is there already some documentation?

Mattias

Well, we actually have such an application (bookkeeping app).

And:
1. No the error will not occur, since they are processing different transactions and therefor different 'documents'.
2. The balance is always calculated on-demand; not stored.

We handle concurrency slightly different. We use System Locks. The first person in a area get the system lock. Anybody else afterwards only sees that area as read-only with a option to send a message to the first user to get out of that area if it is urgent.

We have a lock on the 'document' level. Only one person can edit a document at a time. The second gets a message.

But with large consequences, given our time constraints :-)

For small config needs, I think that JSON is a better choice. It's leight-weight, which is also the very reason it was invented: to counter all SOAP XML bloat.

Well, FPC makes using/handling json particularly easy. Even if I do say so myself ;-) A warning: FPC currently has only ansistring support for JSON. I didn't get around to making it unicode-aware (as the standard actually dictates). But with the new string type, this should be a no-brainer :-)

Michael.

Op 2010-06-22 09:51, Mattias Gaertner het geskryf:

Is there already some documentation?

None that I know of. I looked in the fpcdocs repository.

Regards,
- Graeme -

In our previous episode, Graeme Geldenhuys said:

That is a rather dangerous approach to take. Imagine two accountants have a Clients transaction screen open - both adding new transactions.

You should not be using as of yet unproven components and concepts in accountant apps :)

s/files/threads

Op 2010-06-22 11:17, Henry Vermaak het geskryf:

In tiOPF we have a Singleton for INI file processing. It uses the TMultiReadExclusiveWriteSynchronizer class no manage access, and it is thread-safe.

FPC finally has a working TMultiReadExclusiveWriteSynchronizer implementation (tiOPF implemented it's own), but I don't know if that implementation is available for FPC 2.4.x yet, or if it is still only in Trunk. Either way, TXXXConfig classes could use the same synchronizer to manage access - with a bit of refactoring.

Regards,
- Graeme -

Nono.

We'll put a big warning 'Not concurrency safe' in the files.

Keep it simple. It works for 99% of all use cases. The 1% that need concurrency can use another solution. We should provide this solution, but not in the standard classes.

TStringlist is also not thread safe, and that is OK as long as you know it. Making it thread safe is possible, but would slow it down in most of the cases where it is simply not needed.

Michael.

Op 2010-06-22 11:44, Michael Van Canneyt het geskryf:

Nono. We'll put a big warning 'Not concurrency safe' in the files.

Yes, that is fine, and I understand that. I simply meant it as showing Henry how tiOPF did it for INI files - not mean as FPC's implementation must change as such.

can use another solution. We should provide this solution, but not in the standard classes.

Agreed. I'm going to be playing more with JSON in the coming weeks. I'll work on a thread-safe solution for it (and probably apply that same solution to the XMLConfig package too. I don't like having stacks of .xml or .json files per application. Thread-safe solution will be in a separate unit, in each case.

Regards,
- Graeme -

One of the drawbacks is that it will pull in thread support, and hence the C library. But if you care to donate the code, I will be happy to include it in FPC.

Michael.

Op 2010-06-22 12:01, Michael Van Canneyt het geskryf:

One of the drawbacks is that it will pull in thread support, and hence the C library. But if you care to donate the code, I will be happy to include it in FPC.

I can't remember when last I created an app that wasn't multi-threaded. :-)

Regards,
- Graeme -

Yes, I've done this before, too (in delphi, though). If you add this to the TJSONConfig class, you'll have to keep track of which files are opened by different TJSONConfig classes, then lock them accordingly. This complicates things unnecessarily for a small amount of use cases. I can see how a derived thread-safe class would be handy for some people, but I'd implement this in my app, personally. Much simpler.

Henry

I don't see how this can be useful. Either the first user keeps the file open for writing, so that every other request will fail, or the file is closed after reading, so that any number of writes can follow.

For source code we have several VCS, which try to keep the files consistent, but even these fail to merge multiple changes from time to time. And such systems are quite useless with other (config) data, where little context is available for synchronization attempts - here the XML format provides the most context information, with best chances for successful merges. In either case every update should read the current file contents, and merge it with the local changes before writing the entire file back to disk. Then also writelocks make sense, so that only one update can be applied after another.

DoDi