Issue 286 in protobuf: Inconsistency with repeated fields using C++ protobufs

[prot...@googlecode.com]

Status: New
Owner: liuj...@google.com
Labels: Type-Defect Priority-Medium

New issue 286 by pkwar...@gmail.com: Inconsistency with repeated fields
using C++ protobufs
http://code.google.com/p/protobuf/issues/detail?id=286

What steps will reproduce the problem?
1. Compile the test.proto file attached to this ticket (protoc
--python_out=. test.proto).
2. Run test1.py with PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION=python and then
PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION=cpp.
3. Run test2.py with PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION=python and then
PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION=cpp.

What is the expected output? What do you see instead?
test1.py:
$ PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION=cpp python ./test1.py
libprotobuf FATAL google/protobuf/reflection_ops.cc:51] CHECK failed:
(&from) != (to):
terminate called after throwing an instance
of 'google::protobuf::FatalException'
what(): CHECK failed: (&from) != (to):
Abort trap

$ PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION=python python ./test1.py
repeated {
name: "A"
value: "1"
}

test2.py:
$ PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION=cpp python ./test2.py
repeated {
}

$ PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION=python python ./test2.py
repeated {
name: "B"
value: "4"
}

What version of the product are you using? On what operating system?
protobuf 2.4.1

Please provide any additional information below.

Attachments:
test.proto 138 bytes
test1.py 245 bytes
test2.py 245 bytes

[prot...@googlecode.com]

Comment #1 on issue 286 by jas...@google.com: Inconsistency with repeated

fields using C++ protobufs
http://code.google.com/p/protobuf/issues/detail?id=286

Thanks for reporting this.

I believe what's happening here is some problems with Python refcounting
and the fact that the C++ implementation reuses submessages to avoid
reallocations.

keepitems = [r for r in p.repeated if r.name != 'B']
- this line ends up copying python objects wrapping the pointers to C++
objects stored in the repeated field of messages

del p.repeated[:]
- this line causes the C++ code to clear the repeated field. Under the
hood, the C++ objects are not deleted, but Clear()'ed. The implementation
is unaware that there are still python objects wrapping the C++ objects.

p.repeated.extend(keepitems)
- causes RepeatedPtrField::Add() to be called, which now returns the same
instances as were previously used.

In test1.py, keepitems contains a python object wrapping what's supposed to
be the 'A' object. When extend() is called, the same underlying C++ object
is used, causing the crash, so when it tries to copy the data over using
MergeFrom(), the crash is triggered.

In test2.py, keepitems contains a python object wrapping what's supposed to
be the 'B' object, except that C++ message has been cleared. When extend()
is called, the C++ object originally used to store the 'A' object is
returned. The MergeFrom() call succeeds, but since the message has been
cleared, you get an empty object.

I'm not familiar enough with Python and C extensions to propose a fix, or
even to know how the pure Python implementation handles this. Jisi, any
ideas on that or even just an owner for this bug?

[prot...@googlecode.com]

Updates:
Owner: ---

Comment #2 on issue 286 by jas...@google.com: Inconsistency with repeated

fields using C++ protobufs
http://code.google.com/p/protobuf/issues/detail?id=286

Oh, it turns out this is a known issue, and we have a bug tracking this
internally. Unfortunately, there isn't a lot of available bandwidth to work
out the issues in the python cpp implementation, so progress has been slow.
We'll post updates as they become available.

[prot...@googlecode.com]

Comment #3 on issue 286 by rrbramb...@gmail.com: Inconsistency with

repeated fields using C++ protobufs
http://code.google.com/p/protobuf/issues/detail?id=286

Any update on this issue? It would be great for it to be fixed! This is
causing a lot of problems for us!

[prot...@googlecode.com]

Comment #4 on issue 286 by briss...@gmail.com: Inconsistency with repeated

fields using C++ protobufs
http://code.google.com/p/protobuf/issues/detail?id=286

Same for me, this bug is causing a lot of problems for us. Any fix ?
Regards,
Julien

--
You received this message because this project is configured to send all
issue notifications to this address.
You may adjust your notification preferences at:
https://code.google.com/hosting/settings

[prot...@googlecode.com]

Comment #5 on issue 286 by xiaof...@google.com: Inconsistency with repeated

fields using C++ protobufs
http://code.google.com/p/protobuf/issues/detail?id=286

Internally we have replaced the python c++ implementation with a new one
and the refcounting problem has been addressed in the new design. The code
hasn't been merged into the opensource repository though. It's likely to be
available in the next major release (2.6). As to the current python c++
implementation, we don't plan to do any fix for it as it's supposed to be
deleted in the future.

[prot...@googlecode.com]

Comment #6 on issue 286 by briss...@gmail.com: Inconsistency with repeated

fields using C++ protobufs
http://code.google.com/p/protobuf/issues/detail?id=286

ok thanks for your answer. Have you scheduled a release date ?
With the new implementation, what will be the impact for the end user ?

[prot...@googlecode.com]

Comment #7 on issue 286 by teisenbe...@dropbox.com: Inconsistency with

repeated fields using C++ protobufs
http://code.google.com/p/protobuf/issues/detail?id=286

Any chance you folks have a release date in mind for this yet?

[prot...@googlecode.com]

Comment #8 on issue 286 by xiaof...@google.com: Inconsistency with repeated

fields using C++ protobufs
http://code.google.com/p/protobuf/issues/detail?id=286

Sorry but we don't have a new release on schedule yet. I think the earliest
time the new python C++ implementation can be available is Q1 next year.

[prot...@googlecode.com]

Comment #9 on issue 286 by bram.dej...@native-instruments.de: Inconsistency

with repeated fields using C++ protobufs
http://code.google.com/p/protobuf/issues/detail?id=286

It's already Q2 (almost Q3) 2014. Any update?

[prot...@googlecode.com]

Comment #10 on issue 286 by xiaof...@google.com: Inconsistency with

repeated fields using C++ protobufs
http://code.google.com/p/protobuf/issues/detail?id=286

We are working on a new release (2.6) right now. It will include the new
python c++ implementation and the old experimental one will be removed.
Expected release date is sometime in Q3.

[prot...@googlecode.com]

Comment #11 on issue 286 by robert.e...@gmail.com: Inconsistency with

repeated fields using C++ protobufs
http://code.google.com/p/protobuf/issues/detail?id=286

Do you have a firmer expected release date? Debian is expected to freeze in
Q4 (November 2014), and I'd like to have the new protobuf in the next
stable release of Debian.

(Debian protobuf maintainer here.)

[prot...@googlecode.com]

Updates:
Owner: jie...@google.com

Comment #12 on issue 286 by xiaof...@google.com: Inconsistency with

repeated fields using C++ protobufs
http://code.google.com/p/protobuf/issues/detail?id=286

Hi robert,
I couldn't say for sure when the new release will be out. Our targeted
release date is in the middle of Q3, before the end of August. It's not
guaranteed though.

I added +jieluo@ who has been working on this for some time. She may be
able to give you a more accurate estimation of the release date.

[prot...@googlecode.com]

Updates:
Status: Fixed

Comment #13 on issue 286 by xiaof...@google.com: Inconsistency with

repeated fields using C++ protobufs

https://code.google.com/p/protobuf/issues/detail?id=286

Protobuf releases staring from 2.6.0 should no longer has this problem.