andrewbissada / kryo

Automatically exported from code.google.com/p/kryo
BSD 3-Clause "New" or "Revised" License
0 stars 0 forks source link

Output.setOutputStream() doesn't set the buffer size #111

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
What steps will reproduce the problem?

This is a minimal snippet of something where it made sense to use the same 
Output and to set the OutputStream later.

public class SerTest {
  public static void main(String[] args) {
      Kryo kryo = new Kryo();
      Output out = new Output();
      out.setOutputStream(new ByteArrayOutputStream());

      kryo.writeClassAndObject(out, new String("Somestuff"));
  }
}

What is the expected output? What do you see instead?

I was expecting this code to work. :-)

Apparently, the Output doesn't set the buffer size when you create it with the 
default constructor. The buffer size isn't set if you use 
setOutputStream(OutputStream) and don't use the Output(int bufferSize) 
constructor.

This is what I get with the given example:
Exception in thread "main" com.esotericsoftware.kryo.KryoException: Buffer 
overflow. Max capacity: 0, required: 1
    at com.esotericsoftware.kryo.io.Output.require(Output.java:134)
    at com.esotericsoftware.kryo.io.Output.writeInt(Output.java:242)
    at com.esotericsoftware.kryo.util.DefaultClassResolver.writeClass(DefaultClassResolver.java:84)
    at com.esotericsoftware.kryo.Kryo.writeClass(Kryo.java:472)
    at com.esotericsoftware.kryo.Kryo.writeClassAndObject(Kryo.java:565)
    at SerTest.main(SerTest.java:16)

What version of the Kryo are you using?
2.21

Please provide any additional information below.

Some possible fixes:
 * Set a sensible default buffer size on the default constructor (such as 4096, like what is used when you use Output(Outputstream str));
 * Add to the Javadoc of Output's setOutputStream something like:
    public void setOutputStream(java.io.OutputStream outputStream)
    Sets a new OutputStream. The position and total are reset, discarding any buffered bytes. The Output's internal buffer size isn't set. *Please use the Output(int bufferSize) constructor.*
 * Set the internal buffer size to some sensible value if it is set to 0 when using setOutputStream().

Also, the same problem occurs for Input, for which I propose the same possible 
solutions:
Input input = new Input();
byte[] data = getMyData();
input.setInputStream(new ByteArrayInputStream(data));
kryo.readClassAndObject(input);

Exception in thread "main" com.esotericsoftware.kryo.KryoException: Buffer too 
small: capacity: 0, required: 1
    at com.esotericsoftware.kryo.io.Input.require(Input.java:152)
    at com.esotericsoftware.kryo.io.Input.readInt(Input.java:337)
    at com.esotericsoftware.kryo.util.DefaultClassResolver.readClass(DefaultClassResolver.java:109)
    at com.esotericsoftware.kryo.Kryo.readClass(Kryo.java:610)
    at com.esotericsoftware.kryo.Kryo.readClassAndObject(Kryo.java:721)
    at SerTest.coise(SerTest.java:39)
    at SerTest.main(SerTest.java:18)

Thanks and best regards,
Pitxyoki

Original issue reported on code.google.com by Pitxy...@gmail.com on 28 Apr 2013 at 8:40

GoogleCodeExporter commented 9 years ago
This is a bit of a gotcha. The no arg constructors don't allocate because they 
are for using a buffer that comes from somewhere else or for setting the buffer 
size later. As the javaodcs say, setBuffer must be called before the 
Output/Input can be used. This is true even if using a stream with the buffer.

Original comment by nathan.s...@gmail.com on 28 Apr 2013 at 8:59

GoogleCodeExporter commented 9 years ago
But... If I use the Output(OutputStream) constructor, I don't have to set any 
buffer, as it is automatically created internally.

The issue here is that the API doesn't seem consistent. If I use the 
OutputStream constructor, I don't have to care about buffer sizes at all:
Output out = new Output(new ByteArrayOutputStream());
//(...), later:
out.setOutputstream(new ByteArrayOutputStream());

But if I use the default constructor, I do:
Output out = new Output();
out.setBuffer(new byte[someInt], someInt);
out.setOutputStream(new ByteArrayOutputStream());

I'm (conceptually) writing to a stream, not a byte[]. The Output seems to be 
leaking implementation details when making me set a byte[] when I want to use 
an OutputStream.
When you use the with-OutputStream-constructor, you don't have to care about 
the underlying byte[] of the Output, which is appropriate.

Original comment by Pitxy...@gmail.com on 28 Apr 2013 at 9:33

GoogleCodeExporter commented 9 years ago
The byte[] is not an implementation detail. It is needed even when using an 
OutputStream and can be sized appropriately (eg, small if the total bytes read 
from the OutputStream are likely to be small).

The issue is that the no arg constructor is the only one that constructs an 
Output that is not ready for use. This constructor is intended for when the 
Output does not need its own buffer because it will be writing to a buffer 
provided later.

setOutputStream could allocate a buffer if one was not set, but this side 
effect seems unintuitive. Of course having an Output that can't be used until a 
buffer is set is also unintuitive, but at least it forces you to be explicit 
about allocations.

Original comment by n...@n4te.com on 29 Apr 2013 at 11:31

GoogleCodeExporter commented 9 years ago
"setOutputStream could allocate a buffer if one was not set, but this side 
effect seems unintuitive"
I don't agree. To me, it would seem as natural as the current effects of doing:
Output out = new Output(new ByteArrayOutputStream());

Original comment by Pitxy...@gmail.com on 18 May 2013 at 3:57

GoogleCodeExporter commented 9 years ago
The difference with new Output(stream) is allocation is expected in a 
constructor. I don't think either solution is perfect, but I prefer not 
allocating as a setter side effect.

Original comment by nathan.s...@gmail.com on 5 Aug 2013 at 1:11