You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by "Mathias Herberts (JIRA)" <ji...@apache.org> on 2010/08/31 15:41:53 UTC
[jira] Created: (THRIFT-882) deep copy of binary fields does not
copy ByteBuffer characteristics (arrayOffset, position)
deep copy of binary fields does not copy ByteBuffer characteristics (arrayOffset, position)
-------------------------------------------------------------------------------------------
Key: THRIFT-882
URL: https://issues.apache.org/jira/browse/THRIFT-882
Project: Thrift
Issue Type: Bug
Components: Java - Compiler
Affects Versions: 0.4
Reporter: Mathias Herberts
Generated objects have a constructor which does a deep copy of an existing instance.
For binary fields, the deep copy is done like that:
if (other.isSetBinary_field()) {
this.binary_field = ByteBuffer.wrap(new byte[other.binary_field.limit() - other.binary_field.arrayOffset()]);
System.arraycopy(other.binary_field.array(), other.binary_field.arrayOffset(), binary_field.array(), 0, other.binary_field.limit() - other.binary_field.arrayOffset());
}
This copies the backing array of the ByteBuffer but does not set the position correctly.
In various protocol implementations, ByteBuffer instances are serialized by considering data between position and limit, this means that an object created from another object might lead to different serialized data (and thus a different deserialized value) in case a ByteBuffer has a non default position (which can happen since ByteBuffers can be set).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-882) deep copy of binary fields does not
copy ByteBuffer characteristics (arrayOffset, position)
Posted by "Dave Engberg (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-882?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12905282#action_12905282 ]
Dave Engberg commented on THRIFT-882:
-------------------------------------
Yes, but this level of confusion (by veteran programmers) may show that
the ByteBuffer low-level IO class may not be an ideal general-purpose
data field for user-level generated Java structs. Getting data out of
the ByteBuffer seems a bit confusing and error prone.
I.e. if it were totally up to me, I probably wouldn't expose this in the
default generated Java code for structs produced from an IDL. I'd
either make it an advanced compiler option, or only use it deep in the
hidden engine guts, masked behind getters and setters that return
simpler idempotent data types.
> deep copy of binary fields does not copy ByteBuffer characteristics (arrayOffset, position)
> -------------------------------------------------------------------------------------------
>
> Key: THRIFT-882
> URL: https://issues.apache.org/jira/browse/THRIFT-882
> Project: Thrift
> Issue Type: Bug
> Components: Java - Compiler
> Affects Versions: 0.4
> Reporter: Mathias Herberts
> Assignee: Bryan Duxbury
> Fix For: 0.5
>
> Attachments: thrift-882.patch
>
>
> Generated objects have a constructor which does a deep copy of an existing instance.
> For binary fields, the deep copy is done like that:
> {code}
> if (other.isSetBinary_field()) {
> this.binary_field = ByteBuffer.wrap(new byte[other.binary_field.limit() - other.binary_field.arrayOffset()]);
> System.arraycopy(other.binary_field.array(), other.binary_field.arrayOffset(), binary_field.array(), 0, other.binary_field.limit() - other.binary_field.arrayOffset());
> }
> {code}
> This copies the backing array of the ByteBuffer but does not set the position correctly.
> In various protocol implementations, ByteBuffer instances are serialized by considering data between position and limit, this means that an object created from another object might lead to different serialized data (and thus a different deserialized value) in case a ByteBuffer has a non default position (which can happen since ByteBuffers can be set).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-882) deep copy of binary fields does not
copy ByteBuffer characteristics (arrayOffset, position)
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-882?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12907231#action_12907231 ]
Bryan Duxbury commented on THRIFT-882:
--------------------------------------
I'm coming around to the idea of hiding the ByteBuffers a little. It wouldn't make sense for us to hide them completely, since that would take away basically all of the benefit.
I'm going to open another issue about this.
> deep copy of binary fields does not copy ByteBuffer characteristics (arrayOffset, position)
> -------------------------------------------------------------------------------------------
>
> Key: THRIFT-882
> URL: https://issues.apache.org/jira/browse/THRIFT-882
> Project: Thrift
> Issue Type: Bug
> Components: Java - Compiler
> Affects Versions: 0.4
> Reporter: Mathias Herberts
> Assignee: Bryan Duxbury
> Fix For: 0.5
>
> Attachments: thrift-882.patch
>
>
> Generated objects have a constructor which does a deep copy of an existing instance.
> For binary fields, the deep copy is done like that:
> {code}
> if (other.isSetBinary_field()) {
> this.binary_field = ByteBuffer.wrap(new byte[other.binary_field.limit() - other.binary_field.arrayOffset()]);
> System.arraycopy(other.binary_field.array(), other.binary_field.arrayOffset(), binary_field.array(), 0, other.binary_field.limit() - other.binary_field.arrayOffset());
> }
> {code}
> This copies the backing array of the ByteBuffer but does not set the position correctly.
> In various protocol implementations, ByteBuffer instances are serialized by considering data between position and limit, this means that an object created from another object might lead to different serialized data (and thus a different deserialized value) in case a ByteBuffer has a non default position (which can happen since ByteBuffers can be set).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-882) deep copy of binary fields does not
copy ByteBuffer characteristics (arrayOffset, position)
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-882?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bryan Duxbury updated THRIFT-882:
---------------------------------
Comment: was deleted
(was:
Yes, but this level of confusion (by veteran programmers) may show that
the ByteBuffer low-level IO class may not be an ideal general-purpose
data field for user-level generated Java structs. Getting data out of
the ByteBuffer seems a bit confusing and error prone.
I.e. if it were totally up to me, I probably wouldn't expose this in the
default generated Java code for structs produced from an IDL. I'd
either make it an advanced compiler option, or only use it deep in the
hidden engine guts, masked behind getters and setters that return
simpler idempotent data types.
)
> deep copy of binary fields does not copy ByteBuffer characteristics (arrayOffset, position)
> -------------------------------------------------------------------------------------------
>
> Key: THRIFT-882
> URL: https://issues.apache.org/jira/browse/THRIFT-882
> Project: Thrift
> Issue Type: Bug
> Components: Java - Compiler
> Affects Versions: 0.4
> Reporter: Mathias Herberts
> Assignee: Bryan Duxbury
> Fix For: 0.5
>
> Attachments: thrift-882.patch
>
>
> Generated objects have a constructor which does a deep copy of an existing instance.
> For binary fields, the deep copy is done like that:
> {code}
> if (other.isSetBinary_field()) {
> this.binary_field = ByteBuffer.wrap(new byte[other.binary_field.limit() - other.binary_field.arrayOffset()]);
> System.arraycopy(other.binary_field.array(), other.binary_field.arrayOffset(), binary_field.array(), 0, other.binary_field.limit() - other.binary_field.arrayOffset());
> }
> {code}
> This copies the backing array of the ByteBuffer but does not set the position correctly.
> In various protocol implementations, ByteBuffer instances are serialized by considering data between position and limit, this means that an object created from another object might lead to different serialized data (and thus a different deserialized value) in case a ByteBuffer has a non default position (which can happen since ByteBuffers can be set).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-882) deep copy of binary fields does not
copy ByteBuffer characteristics (arrayOffset, position)
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-882?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bryan Duxbury updated THRIFT-882:
---------------------------------
Assignee: Bryan Duxbury
Fix Version/s: 0.5
> deep copy of binary fields does not copy ByteBuffer characteristics (arrayOffset, position)
> -------------------------------------------------------------------------------------------
>
> Key: THRIFT-882
> URL: https://issues.apache.org/jira/browse/THRIFT-882
> Project: Thrift
> Issue Type: Bug
> Components: Java - Compiler
> Affects Versions: 0.4
> Reporter: Mathias Herberts
> Assignee: Bryan Duxbury
> Fix For: 0.5
>
> Attachments: thrift-882.patch
>
>
> Generated objects have a constructor which does a deep copy of an existing instance.
> For binary fields, the deep copy is done like that:
> {code}
> if (other.isSetBinary_field()) {
> this.binary_field = ByteBuffer.wrap(new byte[other.binary_field.limit() - other.binary_field.arrayOffset()]);
> System.arraycopy(other.binary_field.array(), other.binary_field.arrayOffset(), binary_field.array(), 0, other.binary_field.limit() - other.binary_field.arrayOffset());
> }
> {code}
> This copies the backing array of the ByteBuffer but does not set the position correctly.
> In various protocol implementations, ByteBuffer instances are serialized by considering data between position and limit, this means that an object created from another object might lead to different serialized data (and thus a different deserialized value) in case a ByteBuffer has a non default position (which can happen since ByteBuffers can be set).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-882) deep copy of binary fields does not
copy ByteBuffer characteristics (arrayOffset, position)
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-882?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bryan Duxbury updated THRIFT-882:
---------------------------------
Patch Info: [Patch Available]
> deep copy of binary fields does not copy ByteBuffer characteristics (arrayOffset, position)
> -------------------------------------------------------------------------------------------
>
> Key: THRIFT-882
> URL: https://issues.apache.org/jira/browse/THRIFT-882
> Project: Thrift
> Issue Type: Bug
> Components: Java - Compiler
> Affects Versions: 0.4
> Reporter: Mathias Herberts
> Assignee: Bryan Duxbury
> Fix For: 0.5
>
> Attachments: thrift-882.patch
>
>
> Generated objects have a constructor which does a deep copy of an existing instance.
> For binary fields, the deep copy is done like that:
> {code}
> if (other.isSetBinary_field()) {
> this.binary_field = ByteBuffer.wrap(new byte[other.binary_field.limit() - other.binary_field.arrayOffset()]);
> System.arraycopy(other.binary_field.array(), other.binary_field.arrayOffset(), binary_field.array(), 0, other.binary_field.limit() - other.binary_field.arrayOffset());
> }
> {code}
> This copies the backing array of the ByteBuffer but does not set the position correctly.
> In various protocol implementations, ByteBuffer instances are serialized by considering data between position and limit, this means that an object created from another object might lead to different serialized data (and thus a different deserialized value) in case a ByteBuffer has a non default position (which can happen since ByteBuffers can be set).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-882) deep copy of binary fields does not
copy ByteBuffer characteristics (arrayOffset, position)
Posted by "Mathias Herberts (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-882?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12904944#action_12904944 ]
Mathias Herberts commented on THRIFT-882:
-----------------------------------------
The handling of non array backed buffers will change any existing mark:
orig.mark();
orig.get(copy.array());
orig.reset();
It can be avoided by doing
// Save current position
int pos = orig.position();
int mark = -1;
// Save current mark if set
try {
orig.reset();
mark = orig.position();
orig.position(pos);
} catch (InvalidMarkException ime) {
}
orig.mark();
orig.get(copy.array());
// Get rid of mark and reset position to 0.
orig.rewind();
// Reset mark to its original value if it was set
if (-1 != mark) {
orig.position(mark);
orig.mark();
}
// Reset position to its original value.
orig.position(pos);
> deep copy of binary fields does not copy ByteBuffer characteristics (arrayOffset, position)
> -------------------------------------------------------------------------------------------
>
> Key: THRIFT-882
> URL: https://issues.apache.org/jira/browse/THRIFT-882
> Project: Thrift
> Issue Type: Bug
> Components: Java - Compiler
> Affects Versions: 0.4
> Reporter: Mathias Herberts
> Assignee: Bryan Duxbury
> Fix For: 0.5
>
> Attachments: thrift-882.patch
>
>
> Generated objects have a constructor which does a deep copy of an existing instance.
> For binary fields, the deep copy is done like that:
> {code}
> if (other.isSetBinary_field()) {
> this.binary_field = ByteBuffer.wrap(new byte[other.binary_field.limit() - other.binary_field.arrayOffset()]);
> System.arraycopy(other.binary_field.array(), other.binary_field.arrayOffset(), binary_field.array(), 0, other.binary_field.limit() - other.binary_field.arrayOffset());
> }
> {code}
> This copies the backing array of the ByteBuffer but does not set the position correctly.
> In various protocol implementations, ByteBuffer instances are serialized by considering data between position and limit, this means that an object created from another object might lead to different serialized data (and thus a different deserialized value) in case a ByteBuffer has a non default position (which can happen since ByteBuffers can be set).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-882) deep copy of binary fields does not
copy ByteBuffer characteristics (arrayOffset, position)
Posted by "Mathias Herberts (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-882?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12908905#action_12908905 ]
Mathias Herberts commented on THRIFT-882:
-----------------------------------------
Using ByteBuffer#asReadOnlyBuffer is even safer than #slice.
Otherwise LGTM.
> deep copy of binary fields does not copy ByteBuffer characteristics (arrayOffset, position)
> -------------------------------------------------------------------------------------------
>
> Key: THRIFT-882
> URL: https://issues.apache.org/jira/browse/THRIFT-882
> Project: Thrift
> Issue Type: Bug
> Components: Java - Compiler
> Affects Versions: 0.4
> Reporter: Mathias Herberts
> Assignee: Bryan Duxbury
> Fix For: 0.5
>
> Attachments: thrift-882-v2.patch, thrift-882.patch
>
>
> Generated objects have a constructor which does a deep copy of an existing instance.
> For binary fields, the deep copy is done like that:
> {code}
> if (other.isSetBinary_field()) {
> this.binary_field = ByteBuffer.wrap(new byte[other.binary_field.limit() - other.binary_field.arrayOffset()]);
> System.arraycopy(other.binary_field.array(), other.binary_field.arrayOffset(), binary_field.array(), 0, other.binary_field.limit() - other.binary_field.arrayOffset());
> }
> {code}
> This copies the backing array of the ByteBuffer but does not set the position correctly.
> In various protocol implementations, ByteBuffer instances are serialized by considering data between position and limit, this means that an object created from another object might lead to different serialized data (and thus a different deserialized value) in case a ByteBuffer has a non default position (which can happen since ByteBuffers can be set).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-882) deep copy of binary fields does not
copy ByteBuffer characteristics (arrayOffset, position)
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-882?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12904655#action_12904655 ]
Bryan Duxbury commented on THRIFT-882:
--------------------------------------
I'd love a test case, if you have one.
> deep copy of binary fields does not copy ByteBuffer characteristics (arrayOffset, position)
> -------------------------------------------------------------------------------------------
>
> Key: THRIFT-882
> URL: https://issues.apache.org/jira/browse/THRIFT-882
> Project: Thrift
> Issue Type: Bug
> Components: Java - Compiler
> Affects Versions: 0.4
> Reporter: Mathias Herberts
>
> Generated objects have a constructor which does a deep copy of an existing instance.
> For binary fields, the deep copy is done like that:
> if (other.isSetBinary_field()) {
> this.binary_field = ByteBuffer.wrap(new byte[other.binary_field.limit() - other.binary_field.arrayOffset()]);
> System.arraycopy(other.binary_field.array(), other.binary_field.arrayOffset(), binary_field.array(), 0, other.binary_field.limit() - other.binary_field.arrayOffset());
> }
> This copies the backing array of the ByteBuffer but does not set the position correctly.
> In various protocol implementations, ByteBuffer instances are serialized by considering data between position and limit, this means that an object created from another object might lead to different serialized data (and thus a different deserialized value) in case a ByteBuffer has a non default position (which can happen since ByteBuffers can be set).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-882) deep copy of binary fields does not
copy ByteBuffer characteristics (arrayOffset, position)
Posted by "Dave Engberg (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-882?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12905283#action_12905283 ]
Dave Engberg commented on THRIFT-882:
-------------------------------------
Yes, but this level of confusion (by veteran programmers) may show that
the ByteBuffer low-level IO class may not be an ideal general-purpose
data field for user-level generated Java structs. Getting data out of
the ByteBuffer seems a bit confusing and error prone.
I.e. if it were totally up to me, I probably wouldn't expose this in the
default generated Java code for structs produced from an IDL. I'd
either make it an advanced compiler option, or only use it deep in the
hidden engine guts, masked behind getters and setters that return
simpler idempotent data types.
> deep copy of binary fields does not copy ByteBuffer characteristics (arrayOffset, position)
> -------------------------------------------------------------------------------------------
>
> Key: THRIFT-882
> URL: https://issues.apache.org/jira/browse/THRIFT-882
> Project: Thrift
> Issue Type: Bug
> Components: Java - Compiler
> Affects Versions: 0.4
> Reporter: Mathias Herberts
> Assignee: Bryan Duxbury
> Fix For: 0.5
>
> Attachments: thrift-882.patch
>
>
> Generated objects have a constructor which does a deep copy of an existing instance.
> For binary fields, the deep copy is done like that:
> {code}
> if (other.isSetBinary_field()) {
> this.binary_field = ByteBuffer.wrap(new byte[other.binary_field.limit() - other.binary_field.arrayOffset()]);
> System.arraycopy(other.binary_field.array(), other.binary_field.arrayOffset(), binary_field.array(), 0, other.binary_field.limit() - other.binary_field.arrayOffset());
> }
> {code}
> This copies the backing array of the ByteBuffer but does not set the position correctly.
> In various protocol implementations, ByteBuffer instances are serialized by considering data between position and limit, this means that an object created from another object might lead to different serialized data (and thus a different deserialized value) in case a ByteBuffer has a non default position (which can happen since ByteBuffers can be set).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-882) deep copy of binary fields does not
copy ByteBuffer characteristics (arrayOffset, position)
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-882?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bryan Duxbury updated THRIFT-882:
---------------------------------
Attachment: thrift-882.patch
OK, I had a look at this and came up with this patch. I added a test that I think does a decent job of exercising the cases. I couldn't trivially create a ByteBuffer that had a non-zero arrayOffset(), so I'm not sure when that ever comes into play, but I'm including it for the same of completeness. Additionally, I added support for copying ByteBuffers that have come from other kinds of Channels.
Let me know if this fixes your problem.
> deep copy of binary fields does not copy ByteBuffer characteristics (arrayOffset, position)
> -------------------------------------------------------------------------------------------
>
> Key: THRIFT-882
> URL: https://issues.apache.org/jira/browse/THRIFT-882
> Project: Thrift
> Issue Type: Bug
> Components: Java - Compiler
> Affects Versions: 0.4
> Reporter: Mathias Herberts
> Fix For: 0.5
>
> Attachments: thrift-882.patch
>
>
> Generated objects have a constructor which does a deep copy of an existing instance.
> For binary fields, the deep copy is done like that:
> {code}
> if (other.isSetBinary_field()) {
> this.binary_field = ByteBuffer.wrap(new byte[other.binary_field.limit() - other.binary_field.arrayOffset()]);
> System.arraycopy(other.binary_field.array(), other.binary_field.arrayOffset(), binary_field.array(), 0, other.binary_field.limit() - other.binary_field.arrayOffset());
> }
> {code}
> This copies the backing array of the ByteBuffer but does not set the position correctly.
> In various protocol implementations, ByteBuffer instances are serialized by considering data between position and limit, this means that an object created from another object might lead to different serialized data (and thus a different deserialized value) in case a ByteBuffer has a non default position (which can happen since ByteBuffers can be set).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-882) deep copy of binary fields does not
copy ByteBuffer characteristics (arrayOffset, position)
Posted by "Mathias Herberts (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-882?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12907136#action_12907136 ]
Mathias Herberts commented on THRIFT-882:
-----------------------------------------
I agree with Dave on not exposing ByteBuffer even if they are used internally.
What's others' take on this issue?
> deep copy of binary fields does not copy ByteBuffer characteristics (arrayOffset, position)
> -------------------------------------------------------------------------------------------
>
> Key: THRIFT-882
> URL: https://issues.apache.org/jira/browse/THRIFT-882
> Project: Thrift
> Issue Type: Bug
> Components: Java - Compiler
> Affects Versions: 0.4
> Reporter: Mathias Herberts
> Assignee: Bryan Duxbury
> Fix For: 0.5
>
> Attachments: thrift-882.patch
>
>
> Generated objects have a constructor which does a deep copy of an existing instance.
> For binary fields, the deep copy is done like that:
> {code}
> if (other.isSetBinary_field()) {
> this.binary_field = ByteBuffer.wrap(new byte[other.binary_field.limit() - other.binary_field.arrayOffset()]);
> System.arraycopy(other.binary_field.array(), other.binary_field.arrayOffset(), binary_field.array(), 0, other.binary_field.limit() - other.binary_field.arrayOffset());
> }
> {code}
> This copies the backing array of the ByteBuffer but does not set the position correctly.
> In various protocol implementations, ByteBuffer instances are serialized by considering data between position and limit, this means that an object created from another object might lead to different serialized data (and thus a different deserialized value) in case a ByteBuffer has a non default position (which can happen since ByteBuffers can be set).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Closed: (THRIFT-882) deep copy of binary fields does not
copy ByteBuffer characteristics (arrayOffset, position)
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-882?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bryan Duxbury closed THRIFT-882.
--------------------------------
Resolution: Fixed
I just committed this.
> deep copy of binary fields does not copy ByteBuffer characteristics (arrayOffset, position)
> -------------------------------------------------------------------------------------------
>
> Key: THRIFT-882
> URL: https://issues.apache.org/jira/browse/THRIFT-882
> Project: Thrift
> Issue Type: Bug
> Components: Java - Compiler
> Affects Versions: 0.4
> Reporter: Mathias Herberts
> Assignee: Bryan Duxbury
> Fix For: 0.5
>
> Attachments: thrift-882-v2.patch, thrift-882.patch
>
>
> Generated objects have a constructor which does a deep copy of an existing instance.
> For binary fields, the deep copy is done like that:
> {code}
> if (other.isSetBinary_field()) {
> this.binary_field = ByteBuffer.wrap(new byte[other.binary_field.limit() - other.binary_field.arrayOffset()]);
> System.arraycopy(other.binary_field.array(), other.binary_field.arrayOffset(), binary_field.array(), 0, other.binary_field.limit() - other.binary_field.arrayOffset());
> }
> {code}
> This copies the backing array of the ByteBuffer but does not set the position correctly.
> In various protocol implementations, ByteBuffer instances are serialized by considering data between position and limit, this means that an object created from another object might lead to different serialized data (and thus a different deserialized value) in case a ByteBuffer has a non default position (which can happen since ByteBuffers can be set).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-882) deep copy of binary fields does not
copy ByteBuffer characteristics (arrayOffset, position)
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-882?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12904654#action_12904654 ]
Bryan Duxbury commented on THRIFT-882:
--------------------------------------
It looks like the problem comes from not using the other.binary_field.position() attribute during the array copy, right? This should be a trivial fix.
> deep copy of binary fields does not copy ByteBuffer characteristics (arrayOffset, position)
> -------------------------------------------------------------------------------------------
>
> Key: THRIFT-882
> URL: https://issues.apache.org/jira/browse/THRIFT-882
> Project: Thrift
> Issue Type: Bug
> Components: Java - Compiler
> Affects Versions: 0.4
> Reporter: Mathias Herberts
>
> Generated objects have a constructor which does a deep copy of an existing instance.
> For binary fields, the deep copy is done like that:
> if (other.isSetBinary_field()) {
> this.binary_field = ByteBuffer.wrap(new byte[other.binary_field.limit() - other.binary_field.arrayOffset()]);
> System.arraycopy(other.binary_field.array(), other.binary_field.arrayOffset(), binary_field.array(), 0, other.binary_field.limit() - other.binary_field.arrayOffset());
> }
> This copies the backing array of the ByteBuffer but does not set the position correctly.
> In various protocol implementations, ByteBuffer instances are serialized by considering data between position and limit, this means that an object created from another object might lead to different serialized data (and thus a different deserialized value) in case a ByteBuffer has a non default position (which can happen since ByteBuffers can be set).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-882) deep copy of binary fields does not
copy ByteBuffer characteristics (arrayOffset, position)
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-882?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bryan Duxbury updated THRIFT-882:
---------------------------------
Description:
Generated objects have a constructor which does a deep copy of an existing instance.
For binary fields, the deep copy is done like that:
{code}
if (other.isSetBinary_field()) {
this.binary_field = ByteBuffer.wrap(new byte[other.binary_field.limit() - other.binary_field.arrayOffset()]);
System.arraycopy(other.binary_field.array(), other.binary_field.arrayOffset(), binary_field.array(), 0, other.binary_field.limit() - other.binary_field.arrayOffset());
}
{code}
This copies the backing array of the ByteBuffer but does not set the position correctly.
In various protocol implementations, ByteBuffer instances are serialized by considering data between position and limit, this means that an object created from another object might lead to different serialized data (and thus a different deserialized value) in case a ByteBuffer has a non default position (which can happen since ByteBuffers can be set).
was:
Generated objects have a constructor which does a deep copy of an existing instance.
For binary fields, the deep copy is done like that:
if (other.isSetBinary_field()) {
this.binary_field = ByteBuffer.wrap(new byte[other.binary_field.limit() - other.binary_field.arrayOffset()]);
System.arraycopy(other.binary_field.array(), other.binary_field.arrayOffset(), binary_field.array(), 0, other.binary_field.limit() - other.binary_field.arrayOffset());
}
This copies the backing array of the ByteBuffer but does not set the position correctly.
In various protocol implementations, ByteBuffer instances are serialized by considering data between position and limit, this means that an object created from another object might lead to different serialized data (and thus a different deserialized value) in case a ByteBuffer has a non default position (which can happen since ByteBuffers can be set).
> deep copy of binary fields does not copy ByteBuffer characteristics (arrayOffset, position)
> -------------------------------------------------------------------------------------------
>
> Key: THRIFT-882
> URL: https://issues.apache.org/jira/browse/THRIFT-882
> Project: Thrift
> Issue Type: Bug
> Components: Java - Compiler
> Affects Versions: 0.4
> Reporter: Mathias Herberts
>
> Generated objects have a constructor which does a deep copy of an existing instance.
> For binary fields, the deep copy is done like that:
> {code}
> if (other.isSetBinary_field()) {
> this.binary_field = ByteBuffer.wrap(new byte[other.binary_field.limit() - other.binary_field.arrayOffset()]);
> System.arraycopy(other.binary_field.array(), other.binary_field.arrayOffset(), binary_field.array(), 0, other.binary_field.limit() - other.binary_field.arrayOffset());
> }
> {code}
> This copies the backing array of the ByteBuffer but does not set the position correctly.
> In various protocol implementations, ByteBuffer instances are serialized by considering data between position and limit, this means that an object created from another object might lead to different serialized data (and thus a different deserialized value) in case a ByteBuffer has a non default position (which can happen since ByteBuffers can be set).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
Re: [jira] Commented: (THRIFT-882) deep copy of binary fields does
not copy ByteBuffer characteristics (arrayOffset, position)
Posted by Dave Engberg <de...@evernote.com>.
Yes, but this level of confusion (by veteran programmers) may show that
the ByteBuffer low-level IO class may not be an ideal general-purpose
data field for user-level generated Java structs. Getting data out of
the ByteBuffer seems a bit confusing and error prone.
I.e. if it were totally up to me, I probably wouldn't expose this in the
default generated Java code for structs produced from an IDL. I'd
either make it an advanced compiler option, or only use it deep in the
hidden engine guts, masked behind getters and setters that return
simpler idempotent data types.
On 8/31/10 11:26 PM, Mathias Herberts (JIRA) wrote:
>
> TBaseHelper could be modified to expose a clean way of retrieving a byte[] version of the ByteBuffer's content, this would ease migration to ByteBuffers.
[jira] Commented: (THRIFT-882) deep copy of binary fields does not
copy ByteBuffer characteristics (arrayOffset, position)
Posted by "Mathias Herberts (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-882?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12904946#action_12904946 ]
Mathias Herberts commented on THRIFT-882:
-----------------------------------------
TBaseHelper could be modified to expose a clean way of retrieving a byte[] version of the ByteBuffer's content, this would ease migration to ByteBuffers.
> deep copy of binary fields does not copy ByteBuffer characteristics (arrayOffset, position)
> -------------------------------------------------------------------------------------------
>
> Key: THRIFT-882
> URL: https://issues.apache.org/jira/browse/THRIFT-882
> Project: Thrift
> Issue Type: Bug
> Components: Java - Compiler
> Affects Versions: 0.4
> Reporter: Mathias Herberts
> Assignee: Bryan Duxbury
> Fix For: 0.5
>
> Attachments: thrift-882.patch
>
>
> Generated objects have a constructor which does a deep copy of an existing instance.
> For binary fields, the deep copy is done like that:
> {code}
> if (other.isSetBinary_field()) {
> this.binary_field = ByteBuffer.wrap(new byte[other.binary_field.limit() - other.binary_field.arrayOffset()]);
> System.arraycopy(other.binary_field.array(), other.binary_field.arrayOffset(), binary_field.array(), 0, other.binary_field.limit() - other.binary_field.arrayOffset());
> }
> {code}
> This copies the backing array of the ByteBuffer but does not set the position correctly.
> In various protocol implementations, ByteBuffer instances are serialized by considering data between position and limit, this means that an object created from another object might lead to different serialized data (and thus a different deserialized value) in case a ByteBuffer has a non default position (which can happen since ByteBuffers can be set).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-882) deep copy of binary fields does not
copy ByteBuffer characteristics (arrayOffset, position)
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-882?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bryan Duxbury updated THRIFT-882:
---------------------------------
Comment: was deleted
(was:
Yes, but this level of confusion (by veteran programmers) may show that
the ByteBuffer low-level IO class may not be an ideal general-purpose
data field for user-level generated Java structs. Getting data out of
the ByteBuffer seems a bit confusing and error prone.
I.e. if it were totally up to me, I probably wouldn't expose this in the
default generated Java code for structs produced from an IDL. I'd
either make it an advanced compiler option, or only use it deep in the
hidden engine guts, masked behind getters and setters that return
simpler idempotent data types.
)
> deep copy of binary fields does not copy ByteBuffer characteristics (arrayOffset, position)
> -------------------------------------------------------------------------------------------
>
> Key: THRIFT-882
> URL: https://issues.apache.org/jira/browse/THRIFT-882
> Project: Thrift
> Issue Type: Bug
> Components: Java - Compiler
> Affects Versions: 0.4
> Reporter: Mathias Herberts
> Assignee: Bryan Duxbury
> Fix For: 0.5
>
> Attachments: thrift-882.patch
>
>
> Generated objects have a constructor which does a deep copy of an existing instance.
> For binary fields, the deep copy is done like that:
> {code}
> if (other.isSetBinary_field()) {
> this.binary_field = ByteBuffer.wrap(new byte[other.binary_field.limit() - other.binary_field.arrayOffset()]);
> System.arraycopy(other.binary_field.array(), other.binary_field.arrayOffset(), binary_field.array(), 0, other.binary_field.limit() - other.binary_field.arrayOffset());
> }
> {code}
> This copies the backing array of the ByteBuffer but does not set the position correctly.
> In various protocol implementations, ByteBuffer instances are serialized by considering data between position and limit, this means that an object created from another object might lead to different serialized data (and thus a different deserialized value) in case a ByteBuffer has a non default position (which can happen since ByteBuffers can be set).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Issue Comment Edited: (THRIFT-882) deep copy of binary
fields does not copy ByteBuffer characteristics (arrayOffset, position)
Posted by "Mathias Herberts (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-882?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12904944#action_12904944 ]
Mathias Herberts edited comment on THRIFT-882 at 9/1/10 2:21 AM:
-----------------------------------------------------------------
The handling of non array backed buffers will change any existing mark:
{code}
orig.mark();
orig.get(copy.array());
orig.reset();
{code}
It can be avoided by doing
{code}
// Save current position
int pos = orig.position();
int mark = -1;
// Save current mark if set
try {
orig.reset();
mark = orig.position();
orig.position(pos);
} catch (InvalidMarkException ime) {
}
orig.mark();
orig.get(copy.array());
// Get rid of mark and reset position to 0.
orig.rewind();
// Reset mark to its original value if it was set
if (-1 != mark) {
orig.position(mark);
orig.mark();
}
// Reset position to its original value.
orig.position(pos);
{code}
was (Author: herberts):
The handling of non array backed buffers will change any existing mark:
orig.mark();
orig.get(copy.array());
orig.reset();
It can be avoided by doing
// Save current position
int pos = orig.position();
int mark = -1;
// Save current mark if set
try {
orig.reset();
mark = orig.position();
orig.position(pos);
} catch (InvalidMarkException ime) {
}
orig.mark();
orig.get(copy.array());
// Get rid of mark and reset position to 0.
orig.rewind();
// Reset mark to its original value if it was set
if (-1 != mark) {
orig.position(mark);
orig.mark();
}
// Reset position to its original value.
orig.position(pos);
> deep copy of binary fields does not copy ByteBuffer characteristics (arrayOffset, position)
> -------------------------------------------------------------------------------------------
>
> Key: THRIFT-882
> URL: https://issues.apache.org/jira/browse/THRIFT-882
> Project: Thrift
> Issue Type: Bug
> Components: Java - Compiler
> Affects Versions: 0.4
> Reporter: Mathias Herberts
> Assignee: Bryan Duxbury
> Fix For: 0.5
>
> Attachments: thrift-882.patch
>
>
> Generated objects have a constructor which does a deep copy of an existing instance.
> For binary fields, the deep copy is done like that:
> {code}
> if (other.isSetBinary_field()) {
> this.binary_field = ByteBuffer.wrap(new byte[other.binary_field.limit() - other.binary_field.arrayOffset()]);
> System.arraycopy(other.binary_field.array(), other.binary_field.arrayOffset(), binary_field.array(), 0, other.binary_field.limit() - other.binary_field.arrayOffset());
> }
> {code}
> This copies the backing array of the ByteBuffer but does not set the position correctly.
> In various protocol implementations, ByteBuffer instances are serialized by considering data between position and limit, this means that an object created from another object might lead to different serialized data (and thus a different deserialized value) in case a ByteBuffer has a non default position (which can happen since ByteBuffers can be set).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-882) deep copy of binary fields does not
copy ByteBuffer characteristics (arrayOffset, position)
Posted by "Dave Engberg (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-882?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12905281#action_12905281 ]
Dave Engberg commented on THRIFT-882:
-------------------------------------
Yes, but this level of confusion (by veteran programmers) may show that
the ByteBuffer low-level IO class may not be an ideal general-purpose
data field for user-level generated Java structs. Getting data out of
the ByteBuffer seems a bit confusing and error prone.
I.e. if it were totally up to me, I probably wouldn't expose this in the
default generated Java code for structs produced from an IDL. I'd
either make it an advanced compiler option, or only use it deep in the
hidden engine guts, masked behind getters and setters that return
simpler idempotent data types.
> deep copy of binary fields does not copy ByteBuffer characteristics (arrayOffset, position)
> -------------------------------------------------------------------------------------------
>
> Key: THRIFT-882
> URL: https://issues.apache.org/jira/browse/THRIFT-882
> Project: Thrift
> Issue Type: Bug
> Components: Java - Compiler
> Affects Versions: 0.4
> Reporter: Mathias Herberts
> Assignee: Bryan Duxbury
> Fix For: 0.5
>
> Attachments: thrift-882.patch
>
>
> Generated objects have a constructor which does a deep copy of an existing instance.
> For binary fields, the deep copy is done like that:
> {code}
> if (other.isSetBinary_field()) {
> this.binary_field = ByteBuffer.wrap(new byte[other.binary_field.limit() - other.binary_field.arrayOffset()]);
> System.arraycopy(other.binary_field.array(), other.binary_field.arrayOffset(), binary_field.array(), 0, other.binary_field.limit() - other.binary_field.arrayOffset());
> }
> {code}
> This copies the backing array of the ByteBuffer but does not set the position correctly.
> In various protocol implementations, ByteBuffer instances are serialized by considering data between position and limit, this means that an object created from another object might lead to different serialized data (and thus a different deserialized value) in case a ByteBuffer has a non default position (which can happen since ByteBuffers can be set).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-882) deep copy of binary fields does not
copy ByteBuffer characteristics (arrayOffset, position)
Posted by "Dave Engberg (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-882?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12905280#action_12905280 ]
Dave Engberg commented on THRIFT-882:
-------------------------------------
Yes, but this level of confusion (by veteran programmers) may show that
the ByteBuffer low-level IO class may not be an ideal general-purpose
data field for user-level generated Java structs. Getting data out of
the ByteBuffer seems a bit confusing and error prone.
I.e. if it were totally up to me, I probably wouldn't expose this in the
default generated Java code for structs produced from an IDL. I'd
either make it an advanced compiler option, or only use it deep in the
hidden engine guts, masked behind getters and setters that return
simpler idempotent data types.
> deep copy of binary fields does not copy ByteBuffer characteristics (arrayOffset, position)
> -------------------------------------------------------------------------------------------
>
> Key: THRIFT-882
> URL: https://issues.apache.org/jira/browse/THRIFT-882
> Project: Thrift
> Issue Type: Bug
> Components: Java - Compiler
> Affects Versions: 0.4
> Reporter: Mathias Herberts
> Assignee: Bryan Duxbury
> Fix For: 0.5
>
> Attachments: thrift-882.patch
>
>
> Generated objects have a constructor which does a deep copy of an existing instance.
> For binary fields, the deep copy is done like that:
> {code}
> if (other.isSetBinary_field()) {
> this.binary_field = ByteBuffer.wrap(new byte[other.binary_field.limit() - other.binary_field.arrayOffset()]);
> System.arraycopy(other.binary_field.array(), other.binary_field.arrayOffset(), binary_field.array(), 0, other.binary_field.limit() - other.binary_field.arrayOffset());
> }
> {code}
> This copies the backing array of the ByteBuffer but does not set the position correctly.
> In various protocol implementations, ByteBuffer instances are serialized by considering data between position and limit, this means that an object created from another object might lead to different serialized data (and thus a different deserialized value) in case a ByteBuffer has a non default position (which can happen since ByteBuffers can be set).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-882) deep copy of binary fields does not
copy ByteBuffer characteristics (arrayOffset, position)
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-882?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bryan Duxbury updated THRIFT-882:
---------------------------------
Attachment: thrift-882-v2.patch
I found this snazzy slice() method that makes an independent copy of all the mark/position/etc state without copying the contents which saves me all the trouble of simulating mark().
> deep copy of binary fields does not copy ByteBuffer characteristics (arrayOffset, position)
> -------------------------------------------------------------------------------------------
>
> Key: THRIFT-882
> URL: https://issues.apache.org/jira/browse/THRIFT-882
> Project: Thrift
> Issue Type: Bug
> Components: Java - Compiler
> Affects Versions: 0.4
> Reporter: Mathias Herberts
> Assignee: Bryan Duxbury
> Fix For: 0.5
>
> Attachments: thrift-882-v2.patch, thrift-882.patch
>
>
> Generated objects have a constructor which does a deep copy of an existing instance.
> For binary fields, the deep copy is done like that:
> {code}
> if (other.isSetBinary_field()) {
> this.binary_field = ByteBuffer.wrap(new byte[other.binary_field.limit() - other.binary_field.arrayOffset()]);
> System.arraycopy(other.binary_field.array(), other.binary_field.arrayOffset(), binary_field.array(), 0, other.binary_field.limit() - other.binary_field.arrayOffset());
> }
> {code}
> This copies the backing array of the ByteBuffer but does not set the position correctly.
> In various protocol implementations, ByteBuffer instances are serialized by considering data between position and limit, this means that an object created from another object might lead to different serialized data (and thus a different deserialized value) in case a ByteBuffer has a non default position (which can happen since ByteBuffers can be set).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.