Bug #5176
Buffer field attribute leak
100%
Related issues
History
#1 Updated by Ovidiu Maxiniuc about 3 years ago
- Related to Feature #3574: finish implementation of temp-table XML support added
#2 Updated by Ovidiu Maxiniuc about 3 years ago
I will try to summarize an ugly issue I am investigating lately.
I was working on a issue related to #3574-11 and I discovered that the attribute values for buffer fields are not limited to dynamic temp-tables only. At first I assumed that, since we use the DmoMeta
object as key for storing structures in TableMapper
and instances of the dynamic temp tables share it, only they are affected. My first attempts was to change the static accesses to TableMapper
internal data. The permanent tables did not seem to suffer from this issue, and the static temp-tables. The only solution I imagined was to pass (or extract) somehow the TempTable
object for dynamic temp-tables accessed. The problem is TableMapper
is constructed quite well structured and the interface is almost consistent of the table being queried. Since the permanent tables do not have a parent temp-table, all invocations to TableMapper
methods should have been split in two: one method with current signature (DmoMeta
or DMO Class<>
interface) for permanent tables and one method to pass along the parent temp-table reference to be used as lookup for correct LegacyTableInfo
/ LegacyFieldInfo
. Since there are a lot of these, I abandoned the idea. It is not a good OO code to use conditional to detect the method to be called.
At this moment I tried to approach the problem from another angle. What if I remove the cause for this issue - the shared DMO/Interface? If we are using different LegacyTableInfo
for tables sharing the same DmoMeta
, what would be the penalty for dropping the interface-keyed cache? Some extra CPU cycles for constructing the DMO interface each time. And some memory to store this duplicate objects. It apparently worked at first but then I had the idea of 'hitting' the static temp-tables with similar constructs. There was nothing to suggest any flaw in this regard.
So I constructed a procedure which modifies the field attributes of a static temp-table parameter in called procedure. It looks like this:
/* master.p */
DEFINE VARIABLE hSlave AS HANDLE NO-UNDO.
DEFINE TEMP-TABLE tt FIELD dex AS DECIMAL.
MESSAGE "Outer Before:"
BUFFER tt:BUFFER-FIELD("dex"):DECIMALS
BUFFER tt:BUFFER-FIELD("dex"):SERIALIZE-NAME
BUFFER tt:BUFFER-FIELD("dex"):SERIALIZE-HIDDEN.
RUN slave.p PERSISTENT SET hSlave.
RUN addRecord in hSlave(OUTPUT TABLE tt APPEND).
MESSAGE "Outer After:"
BUFFER tt:BUFFER-FIELD("dex"):DECIMALS
BUFFER tt:BUFFER-FIELD("dex"):SERIALIZE-NAME
BUFFER tt:BUFFER-FIELD("dex"):SERIALIZE-HIDDEN.
/* slave.p */
DEFINE TEMP-TABLE tt FIELD dex AS DECIMAL.
DEFINE TEMP-TABLE tt1 LIKE tt.
PROCEDURE addRecord.
DEFINE OUTPUT PARAMETER TABLE FOR tt1.
CREATE tt.
ASSIGN tt.dex = 3.1415.
CREATE tt1.
BUFFER-COPY tt to tt1.
MESSAGE " Inner Before:"
BUFFER tt:BUFFER-FIELD("dex"):DECIMALS
BUFFER tt:BUFFER-FIELD("dex"):SERIALIZE-NAME
BUFFER tt:BUFFER-FIELD("dex"):SERIALIZE-HIDDEN
BUFFER tt1:BUFFER-FIELD("dex"):DECIMALS
BUFFER tt1:BUFFER-FIELD("dex"):SERIALIZE-NAME
BUFFER tt1:BUFFER-FIELD("dex"):SERIALIZE-HIDDEN.
BUFFER tt:BUFFER-FIELD("dex"):DECIMALS = 5.
BUFFER tt:BUFFER-FIELD("dex"):SERIALIZE-NAME = "meeeh".
BUFFER tt:BUFFER-FIELD("dex"):SERIALIZE-HIDDEN = true.
MESSAGE " Inner After:"
BUFFER tt:BUFFER-FIELD("dex"):DECIMALS
BUFFER tt:BUFFER-FIELD("dex"):SERIALIZE-NAME
BUFFER tt:BUFFER-FIELD("dex"):SERIALIZE-HIDDEN
BUFFER tt1:BUFFER-FIELD("dex"):DECIMALS
BUFFER tt1:BUFFER-FIELD("dex"):SERIALIZE-NAME
BUFFER tt1:BUFFER-FIELD("dex"):SERIALIZE-HIDDEN.
END.
Running master.p
procedure on OE will print:
Outer Before: 0 dex no Inner Before: 0 dex no 0 dex no Inner After: 5 meeeh yes 0 dex no Outer After: 0 dex no
It's evident that the attribute changes affected only the local tt
table in slave.p
procedure. tt1
parameter table is not affected, and neither the tt
table in master.p
.
When running the same piece of code in FWD we get:
Outer Before: 0 dex no Inner Before: 0 dex no 0 dex no Inner After: 5 meeeh yes 0 dex no Outer After: 5 meeeh yes
The changes in local tt
table in slave.p
were picked by the tt
table in master.p
. This is because, like the dynamically constructed temp-table, these also share the same DMO interface so they are mapped to same LegacyTableInfo
/ LegacyFieldInfo
in TableMapper
.
But wait, there is more.
While the dynamic temp-tables which share the same interface are automatically dropped as soon as the user context is dropped (the DynamicTempTableMapper
dynamicDB
is ContextLocal
in TableMapper
), the StaticTempTableMapper
staticTempDB
is just static
. So running the same master.p
again (or in parallel), we get:
Outer Before: 5 meeeh yes Inner Before: 5 meeeh yes 0 dex no Inner After: 5 meeeh yes 0 dex no Outer After: 5 meeeh yes
All three buffer fields attributes (DECIMALS
, SERIALIZE-NAME
, SERIALIZE-HIDDEN
) leaked from a table to another sharing the same interface, but also to another user.
To be honest when I writing the ABL code I was expecting to observe something like: Inner After: 5 meeeh yes 5 meeeh yes
, I.e to see the leak behaving between the tt
and tt1
from server.p
, since the definition of tt1 LIKE tt
would make them having the same structure and logically make them share the same DMO interface. Apparently, this is not the case. For the moment I do not have the answer why this is not happening.
#3 Updated by Greg Shah about 3 years ago
My understanding is that in the 4GL every temp-table instance is a completely separate table. Multiple table instances are created possibly (but not always) from the same definition. Using table parameters is the most common reason for this. This is why they have all the BIND
, APPEND
, BY-REFERENCE
features since by default table parameters cause new instances to be created.
Each of these instances have their own metadata, so changing one will not affect any others even though they were created from the same 4GL code definition.
This approach helps explain why they were so loose with their copying and parameter passing. Since the 4GL syntax never created any mechanism to expose a single definition of a temp-table, each compile unit must have its own internal definitions. Combined with the concept that at runtime, each temp-table is a separate instance with its own metadata, it naturally led to an approach best described as "is this table compatible enough to interoperate". As shown in #3751-492, table parameters are compatible at the structural level (field type, order, number and extent), no table/field names or other options are considered. Two completely different table definitions that have the same structure are completely compatible for passing as parameters. It is certainly a bad/confusing/error prone practice to write code that way but it is the way the 4GL works.
In FWD, we have always been trying to force a different model. It is why we are spending so much time trying to implement TT features because we are not properly matching the 4GL design. For temp-tables, they have no concept of a common DMO. Even for permanent tables, I suspect we have a problem when you consider that in the 4GL, each instance of a database is independently managed. Where we have a single DMO for all permanent database instances.
#4 Updated by Eric Faulhaber about 3 years ago
Ovidiu, if every buffer has its own writable version of a certain set of attributes, does it make sense for each buffer to have its own DmoMeta
instance, and to use the instances we create upon startup as templates for these buffer-specific instances?
#5 Updated by Ovidiu Maxiniuc about 3 years ago
I have really thought of this issue during last days.
The DmoMeta
carries immutable data, that is: information which is used to create the buffer and configure it with the start-up attribute values. If two tables share the same DmoMeta
instance (as far as I know, only related static temp-tables and dynamic temp-tables can do this), they will extract the information from this "prototype" or "template" each time is needed. But the TableMapper.LegacyTableInfo
, TableMapper.LegacyFieldInfo
, etc will store the alterable values for each table instance (regardless of its type). If the attributes are mutable, then FWD will allow to change them, but only for the lifetime interval of the specific table. This hold only for temp-tables, as the attributes of permanent tables are not mutable (AFAIK).
So, not each buffer has a writeable version of the DMO meta data. The permanent database buffers can share the same DmoMeta
, even if they are from different user contexts, but the dynamic temp-table buffer must be really isolated from this POV. The above mentioned TableMapper
structures do exactly that.
Status report:
In my isolated testcase posted in this thread, the issue seems fixed now, I am extending my tests to xfer test suites and later to customer application.
#6 Updated by Ovidiu Maxiniuc about 3 years ago
- Status changed from New to WIP
I committed the fix in 3821c as revision 12108. The update is a aggregate, it contains fixes for other smaller issues. I fixed a few issues which I identified while testing in customer application.
Eric, please do a quick review.
#7 Updated by Eric Faulhaber about 3 years ago
Code review 3821c/12108:
Generally, the changes look good to me, though the change was so dense in TableMapper
, it is difficult to know for sure with just a review. There were also a number of changes outside the persist
package that seemed unrelated to this issue. There are TODOs in BufferFieldImpl.getDefaultString
about not caching transient results. What is doing the caching and what is the plan for these?
Is there anything left to do for this issue?
#9 Updated by Ovidiu Maxiniuc about 3 years ago
Eric Faulhaber wrote:
Code review 3821c/12108:
Generally, the changes look good to me, though the change was so dense inTableMapper
, it is difficult to know for sure with just a review.
The main idea is that all APIs which had a DMO class or a Buffer
as parameter were changed to accept the TempTable
or RecordBuffer
instead. This way the TableMapper
is able to detect the exact LegacyTableInfo
to use/return.
There are some other pieces of code which I think can be replaced by direct delegation to TableMapper
: RecordBuffer.setFieldLabel()
, RecordBuffer.setFieldValidateExpression()
, and related, and the field*
from that RecordBuffer
removed. However, I did not do this because I was afraid the changes would be too radical. There is this comment in RecordBuffer
:7255 about a Progress quirk. I do not know how to test the new code to see whether it would work as expected. There might be some test cases. Nonetheless, I think this support code was written for exactly the same reason I did the changes in TableMapper
.
There were also a number of changes outside the
persist
package that seemed unrelated to this issue.
Yes, the commit contains other fixes, as noted in the comment, most of them related to #5034.
There are TODOs in
BufferFieldImpl.getDefaultString
about not caching transient results. What is doing the caching and what is the plan for these?
Actually I was mislead here. The documentation upholds that the DEFAULT-STRING attribute has Access: Readable/Writeable
. It is not true; from my tests, not even for dynamic temp-tables. But the implementation of getDefaultString
is incorrect. It does not always return the "unformatted version of the INITIAL attribute".
Is there anything left to do for this issue?
No, I think this can be closed. I will eventually create a new thread to track the above issues.
#10 Updated by Greg Shah about 3 years ago
- % Done changed from 0 to 100
- Status changed from WIP to Closed