You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@uima.apache.org by GitBox <gi...@apache.org> on 2020/10/14 11:37:04 UTC

[GitHub] [uima-uimaj] mjunsilo opened a new pull request #66: [UIMA 6273] isBeforeV3 variable not reset in BinaryCasSerDes

mjunsilo opened a new pull request #66:
URL: https://github.com/apache/uima-uimaj/pull/66


   Suggested fix for issue UIMA 6273. This is critical to applications that have mixed loading of v3 and before v3 CAS'es in runtime environments such as the CPE where the CAS objects are recycled with reset.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [uima-uimaj] mjunsilo commented on pull request #66: [UIMA 6273] isBeforeV3 variable not reset in BinaryCasSerDes

Posted by GitBox <gi...@apache.org>.

mjunsilo commented on pull request #66:
URL: https://github.com/apache/uima-uimaj/pull/66#issuecomment-709963108

I actually don't have detailed understanding of this flag, except from what I have picked up from the comments. Marshall introduced it in 2016 with the following original comment "If true, adjust type codes in serialized forms to allow v2 type codes to be correctly read by v3 impls, because v3 impls have a greater number of built-in types". One year later this comment was just reduced to "used to calculate total heap size". The variable is only used in BinaryCasSerDes6.deserializeAfterVersion where it appears to be used in some heap calculations, but my understandings of the low level workings of the CAS are too limited to understand what exactly happens. Obviously there seems to be a difference between v3 and v2 in heap allocation that the deserialization must take account.

As already pointed out, it is currently not vital to clear this flag with the changes I made, and adding unit tests that captures the issue would help. My problem is that I don't know how the use of this flag changes in the future, so I currently feel more safe with the clearing as an extra insurance.

I am not sure I can reproduce the exact same problems as those we had with a small test example. We experienced two different behaviours for v2 and v3 CAS when this flag was incorrectly set. Deserialization would fail with an EOF exception in the case of v2, but it would succeed with v3, except that most annotations were missing.

Currently these problems only occur when you are running an application in the CPE, or similar environment where the CAS is recycled using reset, and both v2 and v3 CAS versions will be loaded during the lifespan of the application. In our case this is an elaborate integration test, and I initially thought the problem was maybe more general until I found the root cause and a fix.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [uima-uimaj] mjunsilo edited a comment on pull request #66: [UIMA 6273] isBeforeV3 variable not reset in BinaryCasSerDes

Posted by GitBox <gi...@apache.org>.

mjunsilo edited a comment on pull request #66:
URL: https://github.com/apache/uima-uimaj/pull/66#issuecomment-708357086


   I am still thinking about how a unit test could be done, but I am short on time at the moment. A proper integrated test requires a solution where a v2 binary is loaded from test resources followed by a CAS reset, load of CAS v3 and final verification of the CAS. Problem is that I have to reproduce the error first with a small enough example before applying the fix, so there can be a fair amount of work to get to that point. The slightly simpler version would be just to check that the isBeforeV3 variable is probably set before and after v3 load, v2 load and reset instead of verifying the whole contents. I also had a quick look at the existing tests like "org.apache.uima.cas.impl.SerDesTest6", but I need to spend more time understanding how they work, before I could say that I could reuse some of that.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [uima-uimaj] mjunsilo commented on pull request #66: [UIMA 6273] isBeforeV3 variable not reset in BinaryCasSerDes

Posted by GitBox <gi...@apache.org>.

mjunsilo commented on pull request #66:
URL: https://github.com/apache/uima-uimaj/pull/66#issuecomment-710766488


   Awesome Richard! I wasn't sure how to approach this exactly myself, and I couldn't find the extra time to mess around with it, but I will note to myself for another time that CasIOUtilsTest and the CasCreationUtils that it uses are very handy helpers for generating CAS test cases.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [uima-uimaj] reckart merged pull request #66: [UIMA 6273] isBeforeV3 variable not reset in BinaryCasSerDes

Posted by GitBox <gi...@apache.org>.

reckart merged pull request #66:
URL: https://github.com/apache/uima-uimaj/pull/66


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [uima-uimaj] mjunsilo edited a comment on pull request #66: [UIMA 6273] isBeforeV3 variable not reset in BinaryCasSerDes

Posted by GitBox <gi...@apache.org>.

mjunsilo edited a comment on pull request #66:
URL: https://github.com/apache/uima-uimaj/pull/66#issuecomment-709963108

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [uima-uimaj] reckart commented on pull request #66: [UIMA 6273] isBeforeV3 variable not reset in BinaryCasSerDes

Posted by GitBox <gi...@apache.org>.

reckart commented on pull request #66:
URL: https://github.com/apache/uima-uimaj/pull/66#issuecomment-708345963


   @mjunsilo Is it feasible to have a unit test for this?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [uima-uimaj] mjunsilo edited a comment on pull request #66: [UIMA 6273] isBeforeV3 variable not reset in BinaryCasSerDes

Posted by GitBox <gi...@apache.org>.

mjunsilo edited a comment on pull request #66:
URL: https://github.com/apache/uima-uimaj/pull/66#issuecomment-708357086


   I am still thinking about how a unit test could be done, but I am short on time at the moment. A proper integrated test requires a solution where a v2 binary is loaded from test resources followed by a CAS reset, load of CAS v3 and final verification of the CAS. Problem is that I have to reproduce the error first with a small enough example before applying the fix, so there can be a fair amount of work to get to that point. The slightly simpler version would be just to check that the isBeforeV3 variable is probably set before and after v3 load, v2 load and reset instead of verifying the whole contents. I also had a quick look at the existing tests like "org.apache.uima.cas.impl.SerDesTest6", but I need to spend more time understanding how they work, before I could reuse some of that.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [uima-uimaj] mjunsilo commented on pull request #66: [UIMA 6273] isBeforeV3 variable not reset in BinaryCasSerDes

Posted by GitBox <gi...@apache.org>.

mjunsilo commented on pull request #66:
URL: https://github.com/apache/uima-uimaj/pull/66#issuecomment-708357086


   I am still thinking about how a unit test could be done, but I am short on time at the moment. A proper integrated test requires a solution where a v2 binary is loaded from test resources followed by a CAS reset, load of CAS v3 and final verification of the CAS. Problem is that I have to reproduce the error first with a small enough example before applying the fix, so there can be a fair amount of work to get to the point. The slightly simpler version would be just to check that the isBeforeV3 variable is probably set before and after v3 load, v2 load and reset instead of verifying the whole contents. I also had a quick look at the existing tests like "org.apache.uima.cas.impl.SerDesTest6", but I need to spend more time understanding how they work, before I could say that I could reuse some of that.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [uima-uimaj] mjunsilo commented on a change in pull request #66: [UIMA 6273] isBeforeV3 variable not reset in BinaryCasSerDes

Posted by GitBox <gi...@apache.org>.

mjunsilo commented on a change in pull request #66:
URL: https://github.com/apache/uima-uimaj/pull/66#discussion_r505908024



##########
File path: uimaj-core/src/main/java/org/apache/uima/cas/impl/BinaryCasSerDes.java
##########
@@ -559,8 +553,10 @@ public SerialFormat reinit(Header h,
       if (!delta) {
         baseCas.resetNoQuestions();
       }
-      
-      
+
+      // isBeforeV3 adjusts binary type numbers, and is reset with the CAS when !delta, so it must be set here.
+      isBeforeV3 = !h.isV3 && h.getSeqVersionNbr() < 2;

Review comment:
       Correct, I had this thought myself, but then I thought that it wouldn't formally be a full reset when the flag isn't cleared. You could also argue that what would happen if someone decided to change this line to the way the assignment was done previously without also clearing the flag during reset? The error would be introduced again, so I think it also adds a bit of robustness to these kind of changes, although it's maybe a bit hypothetic. Anyway, that's why I think it is formally better with the extra clear. Unit tests would of course remove the issue of introducing the error again, and it would then be a pure theoretic discussion about whether it would be a full reset without the clearing.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [uima-uimaj] reckart commented on a change in pull request #66: [UIMA 6273] isBeforeV3 variable not reset in BinaryCasSerDes

Posted by GitBox <gi...@apache.org>.

reckart commented on a change in pull request #66:
URL: https://github.com/apache/uima-uimaj/pull/66#discussion_r505865511



##########
File path: uimaj-core/src/main/java/org/apache/uima/cas/impl/BinaryCasSerDes.java
##########
@@ -559,8 +553,10 @@ public SerialFormat reinit(Header h,
       if (!delta) {
         baseCas.resetNoQuestions();
       }
-      
-      
+
+      // isBeforeV3 adjusts binary type numbers, and is reset with the CAS when !delta, so it must be set here.
+      isBeforeV3 = !h.isV3 && h.getSeqVersionNbr() < 2;

Review comment:
       Isn't this line even enough? Do we need to clear the flag in the reset call?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [uima-uimaj] mjunsilo commented on a change in pull request #66: [UIMA 6273] isBeforeV3 variable not reset in BinaryCasSerDes

Posted by GitBox <gi...@apache.org>.

mjunsilo commented on a change in pull request #66:
URL: https://github.com/apache/uima-uimaj/pull/66#discussion_r505908024



##########
File path: uimaj-core/src/main/java/org/apache/uima/cas/impl/BinaryCasSerDes.java
##########
@@ -559,8 +553,10 @@ public SerialFormat reinit(Header h,
       if (!delta) {
         baseCas.resetNoQuestions();
       }
-      
-      
+
+      // isBeforeV3 adjusts binary type numbers, and is reset with the CAS when !delta, so it must be set here.
+      isBeforeV3 = !h.isV3 && h.getSeqVersionNbr() < 2;

Review comment:
       Correct, I had this thought myself, but then I thought that it wouldn't formally be a full reset when the flag isn't cleared. You could also argue that what would happen if someone decided to change this line to the way the assignment was done previously without also clearing the flag during reset? The error would be introduced again, so I think it also adds a bit of robustness to these kind of changes, although it's maybe a bit hypothetic. Anyway, that's why I think it is formally better with the extra clear.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [uima-uimaj] reckart commented on pull request #66: [UIMA 6273] isBeforeV3 variable not reset in BinaryCasSerDes

Posted by GitBox <gi...@apache.org>.

reckart commented on pull request #66:
URL: https://github.com/apache/uima-uimaj/pull/66#issuecomment-709877797


   @mjunsilo In general, the fix looks good to me. It would be nice if we had a clearer idea under which circumstances not clearing this flag actually causes a problem so we could prep a corresponding CAS with UIMAv2 and bundle it up with a unit test loading this CAS in UIMAv3. I didn't yet look at what the flag actually does control. Do you know what it does?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [uima-uimaj] reckart commented on pull request #66: [UIMA 6273] isBeforeV3 variable not reset in BinaryCasSerDes

Posted by GitBox <gi...@apache.org>.

reckart commented on pull request #66:
URL: https://github.com/apache/uima-uimaj/pull/66#issuecomment-710686113


   @mjunsilo I have generated a few serialized CASes using the UIMAv2 CasIOUtilsTest and create a unit test trying to load these classes in UIMAv3. Without your changes, none of the form-6 files can be loaded. With your change, they load and the number of annotations in the loaded CAS matches the expected number. 


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org