Re: [compress] Added in-memory support for zip and 7z

[Stefan Bodewig]

On 2016-11-01, M N wrote:

>> read never indicates EOF as it stands, I think we should return -1
>> rather than 0 when position equals size. WDYT?

> Yes, indeed the contract specifies to return -1 in this case so we
> should change this.

will do.

>>> In resize() method there is also a danger to overflow integer when
>>> we are close to Integer.MAX_VALUE - either sum pos + wanted or shift
>>> may cause this.

>> True. One fix would be to ensure position + wanted is cut to
>> Integer.MAX_VALUE in write (in case the sum turns negative) and in
>> resize set len to newLength if newLength > Integer.MAX_VALUE / 2.

> Allocation is an interesting topic in itself. I have looked at
> ArrayList allocation algorithm and I think they got it right.

> First there is more conservative approach to allocation growth - 1.5
> ratio instead of 2.0 as we use. Also there is an overflow handled.

> Take a look at ArrayList.grow() method. The question is how to use
> this approach not to violate any license/intellectual property?

Don't copy it. Really.

> In the meantime I have created a patch with additional constructors,
> javadocs and also simple examples for in-memory cases .

Many thanks, I've applied it.

Stefan

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: [compress] Added in-memory support for zip and 7z

[M N]

Hi Stefan,



(...)

> read never indicates EOF as it stands, I think we should return -1
> rather than 0 when position equals size. WDYT?


Yes, indeed the contract specifies to return -1 in this case so we should 
change this.

>> In resize() method there is also a danger to overflow integer when we are 
>> close to Integer.MAX_VALUE - either sum pos + wanted or shift may cause this.

> True. One fix would be to ensure position + wanted is cut to
> Integer.MAX_VALUE in write (in case the sum turns negative) and in
> resize set len to newLength if newLength > Integer.MAX_VALUE / 2.


Allocation is an interesting topic in itself. I have looked at ArrayList 
allocation algorithm and I think they got it right.

First there is more conservative approach to allocation growth - 1.5 ratio 
instead of 2.0 as we use. Also there is an overflow handled.

Take a look at ArrayList.grow() method. The question is how to use this 
approach not to violate any license/intellectual property?


In the meantime I have created a patch with additional constructors, javadocs 
and also simple examples  for in-memory cases .

Patch is attached to Jira issue. Take a look, agree or disagree, thanks.


Cheers,

Maciej


From: Stefan Bodewig 
Sent: Saturday, October 22, 2016 4:46:34 PM
To: dev@commons.apache.org
Subject: Re: [compress] Added in-memory support for zip and 7z

Hi Maciej

patch applied.

On 2016-10-22, M N wrote:

> Going back to the fix - first I've done the homework and read the contract of 
> SeekableByteChannel.position(long) method.

> It influences read() and  write() operation.

> Citation of the most important part:

> "Setting the position to a value that is greater than the current size is 
> legal but does not change the size of the entity. A later attempt to read 
> bytes at such a position will immediately return an end-of-file indication. A 
> later attempt to write bytes at such a position will cause the entity to grow 
> to accommodate the new bytes; the values of any bytes between the previous 
> end-of-file and the newly-written bytes are unspecified."


> My changes to SeekableInMemoryByteChannel:

> 1. In read() method added check whether position exceeds size.

> 2. Added check for negative argument in position(long).

> 3. Added check for open channel in position(long) to fail immediately. In 
> general the contract specifies to throw ClosedChannelExceptionexception by 
> each method.

Thanks, I overlooked 1 and 3 and didn't really think about 2.

read never indicates EOF as it stands, I think we should return -1
rather than 0 when position equals size. WDYT?

> In resize() method there is also a danger to overflow integer when we are 
> close to Integer.MAX_VALUE - either sum pos + wanted or shift may cause this.

True. One fix would be to ensure position + wanted is cut to
Integer.MAX_VALUE in write (in case the sum turns negative) and in
resize set len to newLength if newLength > Integer.MAX_VALUE / 2.

Stefan

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: [compress] Added in-memory support for zip and 7z

[Stefan Bodewig]

Hi Maciej

patch applied.

On 2016-10-22, M N wrote:

> Going back to the fix - first I've done the homework and read the contract of 
> SeekableByteChannel.position(long) method.

> It influences read() and  write() operation.

> Citation of the most important part:

> "Setting the position to a value that is greater than the current size is 
> legal but does not change the size of the entity. A later attempt to read 
> bytes at such a position will immediately return an end-of-file indication. A 
> later attempt to write bytes at such a position will cause the entity to grow 
> to accommodate the new bytes; the values of any bytes between the previous 
> end-of-file and the newly-written bytes are unspecified."


> My changes to SeekableInMemoryByteChannel:

> 1. In read() method added check whether position exceeds size.

> 2. Added check for negative argument in position(long).

> 3. Added check for open channel in position(long) to fail immediately. In 
> general the contract specifies to throw ClosedChannelExceptionexception by 
> each method.

Thanks, I overlooked 1 and 3 and didn't really think about 2.

read never indicates EOF as it stands, I think we should return -1
rather than 0 when position equals size. WDYT?

> In resize() method there is also a danger to overflow integer when we are 
> close to Integer.MAX_VALUE - either sum pos + wanted or shift may cause this.

True. One fix would be to ensure position + wanted is cut to
Integer.MAX_VALUE in write (in case the sum turns negative) and in
resize set len to newLength if newLength > Integer.MAX_VALUE / 2.

Stefan

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: [compress] Added in-memory support for zip and 7z

[M N]

... and patch is attached to Jira COMPRESS-327


From: M N 
Sent: Saturday, October 22, 2016 2:04:53 PM
To: dev@commons.apache.org
Subject: Re: [compress] Added in-memory support for zip and 7z


All,


OK, volatile won't hurt anyway.


Going back to the fix - first I've done the homework and read the contract of 
SeekableByteChannel.position(long) method.

It influences read() and  write() operation.

Citation of the most important part:

"Setting the position to a value that is greater than the current size is legal 
but does not change the size of the entity. A later attempt to read bytes at 
such a position will immediately return an end-of-file indication. A later 
attempt to write bytes at such a position will cause the entity to grow to 
accommodate the new bytes; the values of any bytes between the previous 
end-of-file and the newly-written bytes are unspecified."


My changes to SeekableInMemoryByteChannel:

1. In read() method added check whether position exceeds size.

2. Added check for negative argument in position(long).

3. Added check for open channel in position(long) to fail immediately. In 
general the contract specifies to throw ClosedChannelExceptionexception by each 
method.


In resize() method there is also a danger to overflow integer when we are close 
to Integer.MAX_VALUE - either sum pos + wanted or shift may cause this.


Let me know your thougths.


Cheers,

Maciej


From: Stefan Bodewig 
Sent: Friday, October 21, 2016 4:19:11 AM
To: dev@commons.apache.org
Subject: Re: [compress] Added in-memory support for zip and 7z

On 2016-10-20,  wrote:

> Even when a stream is not thread safe I try at least to make close()
> safe/atomic as aborts and finalizers or shutdown hooks are natural
> sources for concurrency - all using close().

true.

> (However I guess it is less problematic for memory resources)

In the case of SeekableInMemoryByteChannel close really only has effects
on read/write operations, so it may not be that important in this case.

Stefan

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: [compress] Added in-memory support for zip and 7z

[M N]

All,


OK, volatile won't hurt anyway.


Going back to the fix - first I've done the homework and read the contract of 
SeekableByteChannel.position(long) method.

It influences read() and  write() operation.

Citation of the most important part:

"Setting the position to a value that is greater than the current size is legal 
but does not change the size of the entity. A later attempt to read bytes at 
such a position will immediately return an end-of-file indication. A later 
attempt to write bytes at such a position will cause the entity to grow to 
accommodate the new bytes; the values of any bytes between the previous 
end-of-file and the newly-written bytes are unspecified."


My changes to SeekableInMemoryByteChannel:

1. In read() method added check whether position exceeds size.

2. Added check for negative argument in position(long).

3. Added check for open channel in position(long) to fail immediately. In 
general the contract specifies to throw ClosedChannelExceptionexception by each 
method.


In resize() method there is also a danger to overflow integer when we are close 
to Integer.MAX_VALUE - either sum pos + wanted or shift may cause this.


Let me know your thougths.


Cheers,

Maciej


From: Stefan Bodewig 
Sent: Friday, October 21, 2016 4:19:11 AM
To: dev@commons.apache.org
Subject: Re: [compress] Added in-memory support for zip and 7z

On 2016-10-20,  wrote:

> Even when a stream is not thread safe I try at least to make close()
> safe/atomic as aborts and finalizers or shutdown hooks are natural
> sources for concurrency - all using close().

true.

> (However I guess it is less problematic for memory resources)

In the case of SeekableInMemoryByteChannel close really only has effects
on read/write operations, so it may not be that important in this case.

Stefan

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: [compress] Added in-memory support for zip and 7z

[Stefan Bodewig]

On 2016-10-20,  wrote:

> Even when a stream is not thread safe I try at least to make close()
> safe/atomic as aborts and finalizers or shutdown hooks are natural
> sources for concurrency – all using close().

true.

> (However I guess it is less problematic for memory resources)

In the case of SeekableInMemoryByteChannel close really only has effects
on read/write operations, so it may not be that important in this case.

Stefan

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: [compress] Added in-memory support for zip and 7z

[Stefan Bodewig]

On 2016-10-20, M N wrote:

> I have created tests for SeekableInMemoryByteChannel and spot small error.

Great, thanks Maciej.

> Attached is a patch with tests and proposed fix.

The mailing list is set up to strip attachments (at least I don't see
any). Could you attach it to a JIRA issue?

Stefan

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: [compress] Added in-memory support for zip and 7z

[M N]

Hi Stefan,


I have created tests for SeekableInMemoryByteChannel and spot small error.

Attached is a patch with tests and proposed fix.


Regarding thread safety I think would be more clear to remove volatile and 
document a class as not thread safe.

I think the need for thread safe SeekableInMemoryByteChannel will be a rare 
case.


Cheers,

Maciej

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org