Re: autoclosable iterator

[Andy Seaborne]

Another thought: an option is to run the iterator immediately and return 
a disconnected iterator.


Maybe this is good enough for 4.0.0?

It means all the permissions concerns are dealt with , and exception 
generated, at the point of calling the permissions graph. 
PermTripleFilter throws an exception on processing.


Andy

 Sketch
It does "contains" separately as the one obvious case of cost when 
running to completion early.



static class SecuredGraphEx extends GraphBase {
private final Graph dataGraph;
SecuredGraphEx(Graph graph) {
this.dataGraph = graph;
}

@Override
protected boolean graphBaseContains(Triple triplePattern) {
ExtendedIterator eIter = findFilter(triplePattern);
return safeFindAction(eIter, ExtendedIterator::hasNext);
}

@Override
protected ExtendedIterator
   graphBaseFind(Triple triplePattern) {
ExtendedIterator eIter = findFilter(triplePattern);
return materialize(eIter);
}

private ExtendedIterator findFilter(Triple triplePatter) {
ExtendedIterator baseIter = dataGraph.find();
// PermTripleFilter throws AuthenticationRequiredException
Predicate thePermTripleFilter = (t)->true;
return baseIter.filterKeep(thePermTripleFilter);
}

// Execute an action on an iterator, ensuring close is called.
private static  X safeFindAction(ExtendedIterator eIter, 
Function, X> action) {

try {
return action.apply(eIter);
} finally {
eIter.close();
}
}

// Materialize an iterator, ensuring close is called.
private static  ExtendedIterator 
materialize(ExtendedIterator eIter) {

return safeFindAction(eIter, it->{
List x = it.toList();
return WrappedIterator.create(x.iterator());
});
}


On 11/03/2021 22:51, Claude Warren wrote:

My thought in all this was that the try with resources is now standard in
Java so we should try to support it.

Perhaps it makes sense to do the change in V4.
We can clean up our code so that there are no warnings.
Since the issues are warnings only, it should be manageable for our users.

Claude

On Thu, Mar 11, 2021 at 10:48 AM Andy Seaborne  wrote:


I'm not clear which problem we are discussing:

1/ Ensuring inside jena-permissions that iterators are properly closed.

2/ Ensuring use code using jena-permissions (i.e
SecuredGraph/SecuredModel by name) closes iterators properly.

3/ Ensuring app code that uses the Graph or Model interface (and so may
be Secured* but it does not know) closes iterators properly.

If (3) isn't any change going to impact existing use code?

What's more, it does not seem to work

On 10/03/2021 21:56, Claude Warren wrote:

I think that if the solution was any simpler it would have bit me.

Change ClosableIterator to extend AutoCloseable as well as Iterator.
mark ClosableIterator.close() with an @Override annotation and done.

While Autoclosable throws an exception the ClosableIterator does not so

the

try with resources works.

I tried this and then changed
FinderUtils.contains() to be coded as

@Override
  public boolean contains(TriplePattern pattern) {
  try (ClosableIterator it = find(pattern))
  {
  return it.hasNext();
  }
  }

and the IDE (Eclipse) does not complain.  So I think it will just work.


because writing

public boolean contains(TriplePattern pattern) {
ExtendedIterator it = find(pattern);
try {
   return it.hasNext();
} finally { it.close(); }
}

is accident-prone?


I changed ClosableIterator as you describe and got hundreds of warnings
from the codebase, and it will happen to user code.

1/
A warning happens on code that is currently correctly written old style as

  ExtendedIterator<> iter =
  try {
...
  } finally { iter.close(); }

Eclipse "quick-fix" does not do a good job on this.

The other "old style" code is where the iterator runs the the end
(exceptions aside).

2/
Sometimes there isn't a warning!

Warning: as expected:
  public static void main(String...a) throws FileNotFoundException {
  InputStream in = new FileInputStream("");
  }

No warning:  
  static ExtendedIterator foo() { return null; }
  public static void main(String...a) {
  ExtendedIterator iter = foo();
  }

Warning:
  static ExtendedIterator foo() { return null; }
  public static void main(String...a) {
  ExtendedIterator iter = foo();
  iter.close();
  }

Weird.
That somewhat negates the utility.

  Andy

Re: autoclosable iterator

[Andy Seaborne]

AutoCloseable javadoc:

"""
However, when using facilities such as
 * {@link java.util.stream.Stream} that support both I/O-based and
 * non-I/O-based forms, {@code try}-with-resources blocks are in
 * general unnecessary when using non-I/O-based forms.
"""

Andy

On 12/03/2021 12:09, Andy Seaborne wrote:



On 11/03/2021 22:51, Claude Warren wrote:

My thought in all this was that the try with resources is now standard in
Java so we should try to support it.

Re: autoclosable iterator

[Andy Seaborne]

On 11/03/2021 22:51, Claude Warren wrote:

My thought in all this was that the try with resources is now standard in
Java so we should try to support it.


Try-with-resources is primarily for external resources.

Streams are the way for programming.

They are less general that iterators, iterator are very low level - when 
the stream pipeline starts, it executes and the end is known. No single 
stepping.


Hence ExtendedIterator.forEach. (added after a previous close-eIterator 
discussion).


The obvious incremental addition is a default method that is "process 
iterator, return calculation". See "iterRtn" though it feels a bit 
clunky still.


We have use of iterators for "possible multiple results" and how 
multiple property-values are handled is the API question. Closability is 
a tool, not a solution.


Example: Stream.findFirst isn't a pattern to test if there were 
unexpected more than one occurrence.




Perhaps it makes sense to do the change in V4.
We can clean up our code so that there are no warnings.



Since the issues are warnings only, it should be manageable for our users.


As I've said before, I think that tweaking the API has limited benefit.

Starting a new one without needing to be concerned with the incremental 
migration seems to me to be easier.


Streams being one example of a new API.

Andy



Claude

On Thu, Mar 11, 2021 at 10:48 AM Andy Seaborne  wrote:


I'm not clear which problem we are discussing:

1/ Ensuring inside jena-permissions that iterators are properly closed.

2/ Ensuring use code using jena-permissions (i.e
SecuredGraph/SecuredModel by name) closes iterators properly.

3/ Ensuring app code that uses the Graph or Model interface (and so may
be Secured* but it does not know) closes iterators properly.

If (3) isn't any change going to impact existing use code?

What's more, it does not seem to work

On 10/03/2021 21:56, Claude Warren wrote:

I think that if the solution was any simpler it would have bit me.

Change ClosableIterator to extend AutoCloseable as well as Iterator.
mark ClosableIterator.close() with an @Override annotation and done.

While Autoclosable throws an exception the ClosableIterator does not so

the

try with resources works.

I tried this and then changed
FinderUtils.contains() to be coded as

@Override
  public boolean contains(TriplePattern pattern) {
  try (ClosableIterator it = find(pattern))
  {
  return it.hasNext();
  }
  }

and the IDE (Eclipse) does not complain.  So I think it will just work.


because writing

public boolean contains(TriplePattern pattern) {
ExtendedIterator it = find(pattern);
try {
   return it.hasNext();
} finally { it.close(); }
}

is accident-prone?


I changed ClosableIterator as you describe and got hundreds of warnings
from the codebase, and it will happen to user code.

1/
A warning happens on code that is currently correctly written old style as

  ExtendedIterator<> iter =
  try {
...
  } finally { iter.close(); }

Eclipse "quick-fix" does not do a good job on this.

The other "old style" code is where the iterator runs the the end
(exceptions aside).

2/
Sometimes there isn't a warning!

Warning: as expected:
  public static void main(String...a) throws FileNotFoundException {
  InputStream in = new FileInputStream("");
  }

No warning:  
  static ExtendedIterator foo() { return null; }
  public static void main(String...a) {
  ExtendedIterator iter = foo();
  }

Warning:
  static ExtendedIterator foo() { return null; }
  public static void main(String...a) {
  ExtendedIterator iter = foo();
  iter.close();
  }

Weird.
That somewhat negates the utility.

  Andy

Re: autoclosable iterator

[Claude Warren]

My thought in all this was that the try with resources is now standard in
Java so we should try to support it.

Perhaps it makes sense to do the change in V4.
We can clean up our code so that there are no warnings.
Since the issues are warnings only, it should be manageable for our users.

Claude

On Thu, Mar 11, 2021 at 10:48 AM Andy Seaborne  wrote:

> I'm not clear which problem we are discussing:
>
> 1/ Ensuring inside jena-permissions that iterators are properly closed.
>
> 2/ Ensuring use code using jena-permissions (i.e
> SecuredGraph/SecuredModel by name) closes iterators properly.
>
> 3/ Ensuring app code that uses the Graph or Model interface (and so may
> be Secured* but it does not know) closes iterators properly.
>
> If (3) isn't any change going to impact existing use code?
>
> What's more, it does not seem to work
>
> On 10/03/2021 21:56, Claude Warren wrote:
> > I think that if the solution was any simpler it would have bit me.
> >
> > Change ClosableIterator to extend AutoCloseable as well as Iterator.
> > mark ClosableIterator.close() with an @Override annotation and done.
> >
> > While Autoclosable throws an exception the ClosableIterator does not so
> the
> > try with resources works.
> >
> > I tried this and then changed
> > FinderUtils.contains() to be coded as
> >
> > @Override
> >  public boolean contains(TriplePattern pattern) {
> >  try (ClosableIterator it = find(pattern))
> >  {
> >  return it.hasNext();
> >  }
> >  }
> >
> > and the IDE (Eclipse) does not complain.  So I think it will just work.
>
> because writing
>
> public boolean contains(TriplePattern pattern) {
>ExtendedIterator it = find(pattern);
>try {
>   return it.hasNext();
>} finally { it.close(); }
> }
>
> is accident-prone?
>
>
> I changed ClosableIterator as you describe and got hundreds of warnings
> from the codebase, and it will happen to user code.
>
> 1/
> A warning happens on code that is currently correctly written old style as
>
>  ExtendedIterator<> iter =
>  try {
>...
>  } finally { iter.close(); }
>
> Eclipse "quick-fix" does not do a good job on this.
>
> The other "old style" code is where the iterator runs the the end
> (exceptions aside).
>
> 2/
> Sometimes there isn't a warning!
>
> Warning: as expected:
>  public static void main(String...a) throws FileNotFoundException {
>  InputStream in = new FileInputStream("");
>  }
>
> No warning:  
>  static ExtendedIterator foo() { return null; }
>  public static void main(String...a) {
>  ExtendedIterator iter = foo();
>  }
>
> Warning:
>  static ExtendedIterator foo() { return null; }
>  public static void main(String...a) {
>  ExtendedIterator iter = foo();
>  iter.close();
>  }
>
> Weird.
> That somewhat negates the utility.
>
>  Andy
>
>

-- 
I like: Like Like - The likeliest place on the web

LinkedIn: http://www.linkedin.com/in/claudewarren

Re: autoclosable iterator

[Andy Seaborne]

I'm not clear which problem we are discussing:

1/ Ensuring inside jena-permissions that iterators are properly closed.

2/ Ensuring use code using jena-permissions (i.e 
SecuredGraph/SecuredModel by name) closes iterators properly.


3/ Ensuring app code that uses the Graph or Model interface (and so may 
be Secured* but it does not know) closes iterators properly.


If (3) isn't any change going to impact existing use code?

What's more, it does not seem to work

On 10/03/2021 21:56, Claude Warren wrote:

I think that if the solution was any simpler it would have bit me.

Change ClosableIterator to extend AutoCloseable as well as Iterator.
mark ClosableIterator.close() with an @Override annotation and done.

While Autoclosable throws an exception the ClosableIterator does not so the
try with resources works.

I tried this and then changed
FinderUtils.contains() to be coded as

@Override
 public boolean contains(TriplePattern pattern) {
 try (ClosableIterator it = find(pattern))
 {
 return it.hasNext();
 }
 }

and the IDE (Eclipse) does not complain.  So I think it will just work.


because writing

public boolean contains(TriplePattern pattern) {
  ExtendedIterator it = find(pattern);
  try {
 return it.hasNext();
  } finally { it.close(); }
}

is accident-prone?


I changed ClosableIterator as you describe and got hundreds of warnings 
from the codebase, and it will happen to user code.


1/
A warning happens on code that is currently correctly written old style as

ExtendedIterator<> iter =
try {
  ...
} finally { iter.close(); }

Eclipse "quick-fix" does not do a good job on this.

The other "old style" code is where the iterator runs the the end 
(exceptions aside).


2/
Sometimes there isn't a warning!

Warning: as expected:
public static void main(String...a) throws FileNotFoundException {
InputStream in = new FileInputStream("");
}

No warning:  
static ExtendedIterator foo() { return null; }
public static void main(String...a) {
ExtendedIterator iter = foo();
}

Warning:
static ExtendedIterator foo() { return null; }
public static void main(String...a) {
ExtendedIterator iter = foo();
iter.close();
}

Weird.
That somewhat negates the utility.

Andy

Re: autoclosable iterator

[Andy Seaborne]

Streams? There is Stream.onClose.

There are idioms that requires the caller to call close but it does not 
cover the case of a caller returning the ExtendedIterator further up the 
stack.


AutoCloseable does not solve either.

ExtendedIterator.forEach does this for the run-to-completion case and 
run partial case but not if a result is required.


G.getOne* does it by being specific to the operation. For its intended 
use, writing out each of the cases was tedious but better than at the 
graph access points in the SHACL code.


The best for "with result" I came up with is:

public static  X
iterRtn( ExtendedIterator iter,
 Function, X> action) {
try {
return action.apply(iter);
} finally { iter.close(); }
}

and the loop is needed in the function impl:

ExtendedIterator iter = extended iterator of strings 
String x = iterRtn(iter, (eIter)->{
while(iter.hasNext()) {
String t = iter.next();
if ( t.equals("B") )
return "Z";
}
return "";
});


None of which are seamless upgrades.

Andy

On 10/03/2021 21:28, Claude Warren wrote:

In the permissions code I have to call close on all the ExtendedIterators
because I have no idea what is behind them.

I tried to add Autoclosable as an interface using a dynamic proxy in the
hopes that all those iterators that extend ExtendedIterator would still
function properly.  But there was a conflict between the close methods as
you noted.

I was unable to come up with a solution.

Claude


On Sat, Mar 6, 2021 at 8:33 PM Andy Seaborne  wrote:


Could give some details please?

I have tried some things out and they don't give a warning unless close
is explicitly present, unlike InputStream.

Current guess:  because ExtendedIterator.close is not the same as
AutoCloseable.close due to throws.


It's a multi-facet area. Is this primarily for exceptions? Early exit of
a loop? Within jena-permissions - obviously you can do what you think is
the right thing.


What about ExtendedIterator.forEach? That closes on exception.

Have you seen the "org.apache.jena.riot.other.G" library which is
functions to work with graphs that do the close thing.  That was my
solution to making sure lcose was called. We could move (some of) that
to jena-core.

Is there an interaction with "var"?

  Andy

On 06/03/2021 17:56, Claude Warren wrote:

I know that we had discussed adding autoclosable to the iterators but
decided not to.  I find that I am doing a lot of try/finally-close
manipulation with extended iterators.  Rather than rehashing the previous
discussion (I'll assume we made the correct decision before), I would

like

to suggest an AutoClosableExtendedIterator that extends both
ExtendedIterator and AutoClosable.

Is there a reason not to do this?

Claude

Re: autoclosable iterator

[Claude Warren]

I think that if the solution was any simpler it would have bit me.

Change ClosableIterator to extend AutoCloseable as well as Iterator.
mark ClosableIterator.close() with an @Override annotation and done.

While Autoclosable throws an exception the ClosableIterator does not so the
try with resources works.

I tried this and then changed
FinderUtils.contains() to be coded as

@Override
public boolean contains(TriplePattern pattern) {
try (ClosableIterator it = find(pattern))
{
return it.hasNext();
}
}

and the IDE (Eclipse) does not complain.  So I think it will just work.

Re: autoclosable iterator

[Claude Warren]

In the permissions code I have to call close on all the ExtendedIterators
because I have no idea what is behind them.

I tried to add Autoclosable as an interface using a dynamic proxy in the
hopes that all those iterators that extend ExtendedIterator would still
function properly.  But there was a conflict between the close methods as
you noted.

I was unable to come up with a solution.

Claude


On Sat, Mar 6, 2021 at 8:33 PM Andy Seaborne  wrote:

> Could give some details please?
>
> I have tried some things out and they don't give a warning unless close
> is explicitly present, unlike InputStream.
>
> Current guess:  because ExtendedIterator.close is not the same as
> AutoCloseable.close due to throws.
>
>
> It's a multi-facet area. Is this primarily for exceptions? Early exit of
> a loop? Within jena-permissions - obviously you can do what you think is
> the right thing.
>
>
> What about ExtendedIterator.forEach? That closes on exception.
>
> Have you seen the "org.apache.jena.riot.other.G" library which is
> functions to work with graphs that do the close thing.  That was my
> solution to making sure lcose was called. We could move (some of) that
> to jena-core.
>
> Is there an interaction with "var"?
>
>  Andy
>
> On 06/03/2021 17:56, Claude Warren wrote:
> > I know that we had discussed adding autoclosable to the iterators but
> > decided not to.  I find that I am doing a lot of try/finally-close
> > manipulation with extended iterators.  Rather than rehashing the previous
> > discussion (I'll assume we made the correct decision before), I would
> like
> > to suggest an AutoClosableExtendedIterator that extends both
> > ExtendedIterator and AutoClosable.
> >
> > Is there a reason not to do this?
> >
> > Claude
> >
>


-- 
I like: Like Like - The likeliest place on the web

LinkedIn: http://www.linkedin.com/in/claudewarren

Re: autoclosable iterator

[Andy Seaborne]

Could give some details please?

I have tried some things out and they don't give a warning unless close 
is explicitly present, unlike InputStream.


Current guess:  because ExtendedIterator.close is not the same as 
AutoCloseable.close due to throws.



It's a multi-facet area. Is this primarily for exceptions? Early exit of 
a loop? Within jena-permissions - obviously you can do what you think is 
the right thing.



What about ExtendedIterator.forEach? That closes on exception.

Have you seen the "org.apache.jena.riot.other.G" library which is 
functions to work with graphs that do the close thing.  That was my 
solution to making sure lcose was called. We could move (some of) that 
to jena-core.


Is there an interaction with "var"?

Andy

On 06/03/2021 17:56, Claude Warren wrote:

I know that we had discussed adding autoclosable to the iterators but
decided not to.  I find that I am doing a lot of try/finally-close
manipulation with extended iterators.  Rather than rehashing the previous
discussion (I'll assume we made the correct decision before), I would like
to suggest an AutoClosableExtendedIterator that extends both
ExtendedIterator and AutoClosable.

Is there a reason not to do this?

Claude

autoclosable iterator

[Claude Warren]

I know that we had discussed adding autoclosable to the iterators but
decided not to.  I find that I am doing a lot of try/finally-close
manipulation with extended iterators.  Rather than rehashing the previous
discussion (I'll assume we made the correct decision before), I would like
to suggest an AutoClosableExtendedIterator that extends both
ExtendedIterator and AutoClosable.

Is there a reason not to do this?

Claude

-- 
I like: Like Like - The likeliest place on the web

LinkedIn: http://www.linkedin.com/in/claudewarren