Implement the done method on JsonObjectWriter

[asuffield]

This makes JsonObjectWriter fit the ObjectWriter interface, mirroring the behaviour of ProtoWriter.

[BSBandme]

Not sure whether there's any restriction in c++ for adding public API

[haberman]

What is the rationale of this change? These are internal interfaces (inside the internal/ directory) so we don't support having users call these directly.

[asuffield]

It's used extensively in https://github.com/grpc-ecosystem/grpc-httpjson-transcoding (sad but true) and I have a change to that which wants to be able to use a JsonObjectWriter as an ObjectWriter.

[asuffield]

ping?

[acozzette]

I can take over reviewing since Josh is on vacation.

[haberman]

I am still not a fan of making changes to internal-only interfaces to support users who are calling them directly.

These interfaces could (and in my opinion, should) disappear completely in a future release. This is way more API surface area than we want to be supporting officially.

Why is the transcoding package using these interfaces and how can they be migrated off?

[asuffield]

@haberman I think you would have to talk to the grpc team about that. I can't check these days, but I'm guessing the answer is on the google3 side (although it might just be http://www.hyrumslaw.com/)

I do note that you have //visibility:public on all headers, so it's never really been internal-only and will probably need an LSC to change that:

protobuf/BUILD

Lines 222 to 229 in d2d6ff5

	hdrs = glob([
	"src/**/*.h",
	"src/**/*.inc",
	]),
	copts = COPTS,
	includes = ["src/"],
	linkopts = LINK_OPTS,
	visibility = ["//visibility:public"],

[acozzette]

@haberman I suspect that JsonObjectWriter is actually supposed to be public. Within Google it's in a subdirectory called public and I can't figure out any reason why the open-source version is an internal directory.

[acozzette]

@asuffield The failing test is reporting some memory leaks in the new test cases. I don't see any obvious leak but do you have any ideas what the problem is?

[haberman]

I suspect that JsonObjectWriter is actually supposed to be public. Within Google it's in a subdirectory called public and I can't figure out any reason why the open-source version is an internal directory.

I strongly disagree. This code was only open-sourced to support the public functionality in https://github.com/protocolbuffers/protobuf/blob/master/src/google/protobuf/util/json_util.h

We (as the protobuf team) wanted to open-source JSON <-> protobuf functions for proto3. These are simple string->string functions (JsonToBinaryString(), BinaryToJsonStream(), etc.). Those are the only public interfaces.

Internally these functions use the ObjectWriter framework, so we had to open-source that code also. However we did not publicly expose those interfaces -- that's why they are in internal/. It is a much larger API surface area with much more subtlety (as this PR illustrates).

We are very careful about what interfaces we publicly expose (in open-source), because our burden of backward compatibility on public interfaces is high. Inside Google we can use LSC's to make backward-incompatible changes, but for our open-source code we do not have that luxury. So for public interfaces, we essentially commit to never breaking backward-compatibility. This is why we must be more selective about what interfaces we publicly expose in open-source releases.

I think it would be a grave mistake to sign up for this level of support for these interfaces. Especially since we aren't the authors or primary maintainers of this code. The people who change this code inside google3 are probably not thinking about open-source compatibility when they make changes.

I strongly object to elevating these to public interfaces. They have never been public, and if anything we should make it more clear that they are not public.

I do note that you have //visibility:public on all headers, so it's never really been internal-only and will probably need an LSC to change that

Bazel visibility lists have never been the primary determining factor about what is private. Ideally we would clean BUILD files up to denote visibility better, but the vast majority of our users don't use Bazel anyway. The canonical way of knowing what is private is what is in the internal:: namespace or has internal/ in the header name. Anything with "internal" in its name is subject to breakage when we release new versions.

[acozzette]

@haberman That makes sense that this code might have been open-sourced only for the purpose of supporting proto3 JSON and it was intended to be private. Where does that leave us with respect to this pull request, though? This PR doesn't introduce any new APIs and is arguably just a bug fix for JsonObjectWriter.

[haberman]

We should file a bug on https://github.com/grpc-ecosystem/grpc-httpjson-transcoding that they are using private interfaces. We need a long-term plan for resolving this.

If we have that, then I think we can accept a bugfix like this.

[asuffield]

@acozzette Ah. Eventually I spotted it. Every test in this file leaks memory: they all call new at the start and never delete it.

I guess the CI check doesn't count pre-existing leaks. I changed the new tests to allocate on the stack, we'll see if that makes it happier.

[acozzette]

@asuffield I don't think that explains it because the test fixture destructor (~JsonObjectWriterTest) should be taking care of deleting those objects. Also I'm not aware of any mechanism for our leak checking to ignore pre-existing leaks. I kicked off the tests again, though, so we can see what happens.

[asuffield]

@acozzette Ah yes, you are correct (removed the pointless change). Much head-scratching later, I think I found a smoking gun:

$ docker run protobuftesting/cpp_tcmalloc_79238ab7e47d0089d56b4a9e25e62ecf48b2e340 dpkg -l libgoogle-perftools4
Desired=Unknown/Install/Remove/Purge/Hold
| Status=Not/Inst/Conf-files/Unpacked/halF-conf/Half-inst/trig-aWait/Trig-pend
|/ Err?=(none)/Reinst-required (Status,Err: uppercase=bad)
||/ Name                 Version      Architecture Description
+++-====================-============-============-============================================================================
ii  libgoogle-perftools4 2.2.1-0.2    amd64        libraries for CPU and heap analysis, plus an efficient thread-caching malloc

That is a version of tcmalloc from 2015. When I run the same build using tcmalloc 2.5, there are no errors. I'm guessing that we're looking at a tcmalloc bug from the past. Please prod somebody to rebuild the kokoro docker images.

[acozzette]

@asuffield OK, thanks for tracking that down. But let's not worry too much about the tcmalloc problem for now because I think the more immediate issue is @haberman's concerns above about external projects using this private API. Have you tried reaching out to the grpc-httpjson-transcoding project to see if they can commit to moving off this API in the future?

[acozzette]

Let me go ahead and close this since it sounds like we never really reached an agreement on using these internal-only interfaces.