Page 1 of 4

Issue #22: HostFiles.NewWriter violating the specification

Posted: Sun Mar 16, 2014 1:13 pm
by Ivan Denisov
Finder & Solver: Ilya Ermakov

According the specification in a case of read-only files NewWriter should return NIL.
System/Docu/Files wrote:PROCEDURE (f: File) NewWriter (old: Writer): Writer
NEW, ABSTRACT
Returns a writer which has the appropriate type (for this file type). If old = NIL, then a new writer is allocated. If old # NIL and old has the appropriate type, old is returned. Otherwise, a new writer is allocated. The returned writer is connected to f, and its position is somewhere on the file. If an old writer is passed as parameter, the old position will be retained if possible.
If an old writer is passed as parameter, it is the application's responsibility to guarantee that it is not in use anymore. Passing an unused old writer is recommended because it avoids unnecessary allocations.
Read-only files allow no writers at all. In such cases, NewWriter returns NIL.

Post
result # NIL
old # NIL & old.Base() = f
result.Pos() = old.Pos()
old = NIL OR old.Base() # f
result.Pos() = f.Length()
result = NIL
read-only file
However the code of HostFiles is next:

Code: Select all

       PROCEDURE (f: File) NewWriter (old: Files.Writer): Files.Writer;
          VAR w: Writer;
       BEGIN   (* portable *)
          ASSERT(f.state # closed, 20); ASSERT(f.state # shared, 21);
          IF (old # NIL) & (old IS Writer) THEN w := old(Writer) ELSE NEW(w) END;
          IF w.base # f THEN
             w.base := f; w.buf := NIL; w.SetPos(f.len)
          END;
          RETURN w
       END NewWriter;
ASSERT(f.state # shared, 21); means TRAP except RETURN NIL.

Ilya had suggested next solution:

Code: Select all

   PROCEDURE (f: File) NewWriter (old: Files.Writer): Files.Writer;
      VAR w: Writer;
   BEGIN   (* portable *)
      ASSERT(f.state # closed, 20);
      IF f.state # shared THEN
         IF (old # NIL) & (old IS Writer) THEN w := old(Writer) ELSE NEW(w) END;
         IF w.base # f THEN
            w.base := f; w.buf := NIL; w.SetPos(f.len)
         END;
         RETURN w
      ELSE
         RETURN NIL
      END
   END NewWriter;
Lets vote for including this fix in the Center version.

Re: B1: HostFiles.NewWriter is violating the specification

Posted: Thu Apr 17, 2014 1:03 am
by Ivan Denisov
I am really think, that we should vote for each change in the forum like this example.

The Center task is provide stable version and select stable features to unite users around authoritative BlackBox version. It is impossible without deep analysis and voting around each change.

Re: B1: HostFiles.NewWriter is violating the specification

Posted: Thu Apr 17, 2014 7:48 am
by ReneK
While, yes, each bug needs to be analsed and fixed with scrutiny, I am not sure that voting for each and every bug fix is manageable.

Re: B1: HostFiles.NewWriter is violating the specification

Posted: Fri Apr 18, 2014 8:39 am
by Josef Templ
I agree with Rene.

We have a backlog of more than 50 changes and if we are not dealing
with this backlog quickly, the new issues will arrive faster than the backlog shrinks,
which leads to frustration.

In addition, having looked at all the changes done in the CPC 1.7 edition, I know
that it is a lot of work and it will not be possible for most of the center members
to analyze all the changes in depth, both for the skills and for the time required.
You will have to dig deeply into the compiler, for example.
Some of the changes are of course rather simple.

For those center members who have both the skills and the time I would
propose they look at the CPC 1.7 edition and if there are any problems with
the existing fixes, please list them.
I would expect this list to be a rather small.

The main problem with the CPC edition, In my opinion, is that it is
not yet put in a (git) repository with an issue tracker being connected.
Instead, it uses a text file for describing all the changes.
So this would be my recommended first steps.
0. set up a repository and an issue tracker
1. put the 'BB 1.6 final' sources and resources in a repository (not the binary files)
2. add the CPC 1.7 issues one by one to an issue tracker (such as redmine)
and commit the changes related to each issue individually to the repository.
Even this 'editing' is quite some work, but not comparable with the work
already being done.
After this work has been done, it will be much easier for
the center members to look at the list of resolved issues and how they have
been resolved. One by One.

Ideally there should also be a build machine that automates the
creation of a downloadable binary file after changes in the repository.
But this is a separate issue that can be worked on in parallel.
It can be done manually at the beginning. Later it should be automated.
One tool dedicated to exactly this problem is Jenkins.
There may be other tools or simpler ones, I don't know.
Jenkins seems to be one of the standard open source tools used
by many similar projects for what is called 'continuous integration',
which means to continually integrate the changes committed to a
repository into a binary distribution package.
Then you get features like latest stable build, daily build,
latest developer build, statistics, error logs, etc.

- Josef

Re: B1: HostFiles.NewWriter is violating the specification

Posted: Fri Apr 18, 2014 1:14 pm
by cfbsoftware
There is also the strong possibility that the code is as intended but the documentation was mistakenly not updated when the code was changed. An earlier version of the source code was:

Code: Select all

   PROCEDURE (f: File) NewWriter (old: Files.Writer): Files.Writer;
		VAR w: Writer;
	BEGIN	(* portable *)
		ASSERT(f.state # closed, 20);
		IF f.state IN {shared, shidden} THEN RETURN NIL END;
		IF (old # NIL) & (old IS Writer) THEN w := old(Writer) ELSE NEW(w) END;
		IF w.base # f THEN
			w.base := f; w.buf := NIL; w.SetPos(f.len)
		END;
		RETURN w
	END NewWriter;
The assertion appears to have been intentionally added. This highlights the error as soon as it is detected, rather than letting a NIL reference cause a problem sometime later.

Re: B1: HostFiles.NewWriter is violating the specification

Posted: Fri Apr 18, 2014 2:02 pm
by Josef Templ
cfbsoftware wrote:There is also the strong possibility that the code is as intended but the documentation was mistakenly not updated when the code was changed. An earlier version of the source code was:

Code: Select all

   PROCEDURE (f: File) NewWriter (old: Files.Writer): Files.Writer;
		VAR w: Writer;
	BEGIN	(* portable *)
		ASSERT(f.state # closed, 20);
		IF f.state IN {shared, shidden} THEN RETURN NIL END;
		IF (old # NIL) & (old IS Writer) THEN w := old(Writer) ELSE NEW(w) END;
		IF w.base # f THEN
			w.base := f; w.buf := NIL; w.SetPos(f.len)
		END;
		RETURN w
	END NewWriter;
The assertion appears to have been intentionally added. This highlights the error as soon as it is detected, rather than letting a NIL reference cause a problem sometime later.

This is a very strong point indeed.
BTW, where did you get this previous code version from?
In 1.5 and 1.5 Beta I cannot find this code.
Until 1.4 there was no official release of the source code, was it?

In any case, it really gives sense to trap as soon as possible.
I have changed my vote to 'no', which means that we should change the documentation.
@Helmut: what do you think?

- Josef

Re: B1: HostFiles.NewWriter is violating the specification

Posted: Fri Apr 18, 2014 6:00 pm
by Ivan Denisov
The argument was, that there is no way to now is there file shared or not with a help of framework. That is why this TRAP becomes unavoidable.

There are one rule in Files documentation.
Opening a file in shared mode is the rule in BlackBox; opening a file in exclusive mode is an infrequent exception.
So, this problem should not appear if you led this rule.

However, how to prevent TRAP in infrequent exceptions? That means that programmer should provide this check with it's own module variables... and make strong encapsulation of files variables.

One File can be open with dir.Old other made with dir.New. You can not distinguish can you make file.NewWriter or not by having only the Files.File variable.

Re: B1: HostFiles.NewWriter is violating the specification

Posted: Fri Apr 18, 2014 6:20 pm
by Ivan Denisov
The other solution is to make new procedure in Files.

Code: Select all

PROCEDURE (f: File) IsWritable* (): BOLEAN, NEW, ABSTRACT;
And it's simple realisation in HostFiles.

Code: Select all

PROCEDURE (f: File) IsWritable* (): BOLEAN;
BEGIN
RETURN (f.state # shared) & (f.state # closed)
END Shared;

Re: B1: HostFiles.NewWriter is violating the specification

Posted: Fri Apr 18, 2014 11:56 pm
by DGDanforth
Josef Templ wrote:I agree with Rene.

We have a backlog of more than 50 changes and if we are not dealing
with this backlog quickly, the new issues will arrive faster than the backlog shrinks,
which leads to frustration....

- Josef
That seems like a very good plan to me and I support it. Now how to make it happen?

Re: B1: HostFiles.NewWriter is violating the specification

Posted: Sat Apr 19, 2014 2:07 am
by Ivan Denisov
DGDanforth wrote:
Josef Templ wrote:I agree with Rene.

We have a backlog of more than 50 changes and if we are not dealing
with this backlog quickly, the new issues will arrive faster than the backlog shrinks,
which leads to frustration....

- Josef
That seems like a very good plan to me and I support it. Now how to make it happen?
This experience with HostFiles shows that each bugfix should be discussed. Doug, for managing this "more than 50 changes" you should add the topic for each change for discussion on the forum happen, after the discussion we will put it on the issues tracker if a format of (issue + solution) and later commit this changes in downloadable repository.