Re: [jgit-dev] nio exception

[Shawn Pearce <spearce@xxxxxxxxxxx>]

On Fri, Mar 9, 2012 at 07:33, Markus Duft <markus.duft@xxxxxxxxxx> wrote:
> On 03/09/2012 04:28 PM, Shawn Pearce wrote:
>> On Thu, Mar 8, 2012 at 22:57, Markus Duft <markus.duft@xxxxxxxxxx> wrote:
>>> On 03/07/2012 04:30 PM, Shawn Pearce wrote:
>>>> On Tue, Mar 6, 2012 at 23:17, Markus Duft <markus.duft@xxxxxxxxxx> wrote:
>>>>> On 03/07/2012 03:32 AM, Shawn Pearce wrote:
>>>>>> On Tue, Mar 6, 2012 at 05:16, Markus Duft <markus.duft@xxxxxxxxxx> wrote:
>>>>> [snip]
>>> [snip]
>>>>> problematic here is the in.getChannel().size()... this uses a NIO channel to determine the size of the file (lol). This is the step that can fail. I'm not really sure what would be the best solution here, but i think a simple "final long sz = path.length();" would do instead. or is File.length() somehow bad? or more bad than Channel.size()?
>>>>
>>>> Whoops. The problem with path.length() is the inode could have changed
>>>> out from under us between when the file was opened, and when the size
>>>> was estimated. The only safe way to know is to use the channel to get
>>>> at fstat() in the system, or to use the path.length() as an estimate
>>>> for the buffer size but be prepared to grow or shrink our buffer if
>>>> the actual file length is larger or smaller, as determined by when
>>>> read() returns the EOF indicator.  :-)
>>>
>>> are you really sure this is a problem? i checked the openjdk implementation of FileSystem.getLength() (which is used by File.length()), and it does a stat() on the path always. this should be for sure sufficient to not confuse anything :) it will also catch the case where inodes change. nothing is cached. nowhere. if there are java implementations that do cache - i think, _thats_ the bug then ;p
>>
>> Yes, it is a problem:
>>
>>   Thread 1:
>>
>>     FileInputStream in = new FileInputStream(....);
>>
>>   Thread 2:
>>
>>     echo hi >config.new; mv config.new config
>>
>>   Thread 1:
>>       long len = path.length();  // now 3 instead of say 4000.
>
> are you serious?

Yes, completely.

> you have to rewrite the file in between two code lines. you'll have to hit a nanoseconds timeframe?!

Yes. Its not nanoseconds. Modern systems are multi-tasking time
slicing things. Our thread can be put to sleep, and the CPU turned
over to the user's `git config` command they have running in a shell.
If that changes the configuration before our thread gets back onto the
CPU, which is entirely possible as the slice could be 100ms worth of
time and there may be a lot more running on this system than just the
thing running JGit... the file can change length and now our code may
misread the file.

> other than that i think this is a little overkill,

No it isn't. We try to write a reliable library, not a library that
works 90% of the time. I run JGit inside of servers that are up
24/7/365. It should always be safe to (safely according to Git rules)
modify files while the server is running and have the right thing
happen.

> how about calling length() twice, once before, once after and then taking max() of the two ... ? :D

No, it doesn't work. What if the buffer isn't completely populated
because the file shrank?

>the "be prepared to change buffer sizes" version sounds much harder to implement. any other ideas?

You can estimate the buffer size by getting path.length() after you
open the file. It will most likely be correct. If you find EOF
earlier, make a new smaller array and copy the valid region. If you
run out of buffer space before EOF, allocate a bigger buffer, read
more data, and then when you find EOF, make a smaller array and copy
if you need to. Its not that bad. Something like this?

  byte[] buf = new byte[(int) Math.min(path.length(), Integer.MAX_VALUE)];
  int valid = 0;
  for (;;) {
    if (buf.length == valid) {
      byte[] nb = new byte[buf.length * 2];
      System.arraycopy(buf, 0, nb, 0, valid);
      buf = nb;
   }
    int n = in.read(buf, valid, buf.length - valid);
    if (n < 0)
      break;
    valid += n;
  }
  if (valid < buf.length) {
    byte[] nb = new byte[valid];
    System.arraycopy(buf, 0, nb, 0, valid);
    buf = nb;
  }