Different behaviour between Panache repository.list() and repository.stream() methods

[Loïc MATHIEU]

Hello,

I discover a different behaviour between the list() and stream() methods of Panache repository. I don't know if it's an issue or me that didn't use them correctly ;)

When using list, the following method works :

public List<PrepaidCard> list(String language) {
    return prepaidCardRepository.list("statusId != 'DIS'").stream()
            .map(prepaidCard -> addStatusLabel(prepaidCard, language))
            .collect(Collectors.toList());
}

Note here that the addStatusLabel method is doing a SELECT query on the database.

When I use the stream() shorthand method instead of list().stream(), I have the following error on Hibernate :

org.hibernate.exception.GenericJDBCException: could not advance using next()

[...]

Caused by: java.sql.SQLException: ResultSet is closed
at io.agroal.pool.wrapper.ResultSetWrapper$1.invoke(ResultSetWrapper.java:49)
at com.sun.proxy.$Proxy82.next(Unknown Source)
at io.agroal.pool.wrapper.ResultSetWrapper.next(ResultSetWrapper.java:85)
at org.hibernate.internal.ScrollableResultsImpl.next(ScrollableResultsImpl.java:99)
... 129 more

I need to add a @Transactional annotation for this to work.

@Transactional()
public List<PrepaidCard> list(String language) {
    return prepaidCardRepository.stream("statusId != 'DIS'")
            .map(prepaidCard -> addStatusLabel(prepaidCard, language))
            .collect(Collectors.toList());
}

Apparently, when using stream() method, if I make another database call, the cursor for the main query is closed except when I add a @Transactional annotation, so except when I open a transaction that will keep the first cursor open. I don't know if it's a bug or if it's the normal behaviour. But openning a transaction for running SELECT queries don't seems to be a good things to do mainly as we don't have readOnly settings by transaction ...

Regards,

Loïc

[Emmanuel Bernard]

I suppose the stream operator uses the scrollable result set and therefore cursors. These do require an open database transaction. I don’t know how a nested query on the same connection influences that outside a transaction. 

List().stream() is eager. .stream() is not. That’s a big (valuable) difference. 

Emmanuel 

--
You received this message because you are subscribed to the Google Groups "Quarkus Development mailing list" group.
To unsubscribe from this group and stop receiving emails from it, send an email to quarkus-dev...@googlegroups.com.
Visit this group at https://groups.google.com/group/quarkus-dev.
To view this discussion on the web visit https://groups.google.com/d/msgid/quarkus-dev/CAJLxjVEnEDn%3DXU9xbcSaGDzTYaHVhhqHkgH3KDkBNr5ZJbucKw%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

[Loïc MATHIEU]

Hi,

Thanks for the reply, so this is normal behaviour indeed ...

FYI, if I didn't use any nested query, the stream() operator works without an `@Transactional` annotation.

If using the stream() operator mandates a transaction, maybe we need to:

- document it, in the Javadoc at least because it incorrectly states "This method is a shortcut for <code>find(query, sort, params).stream()</code>." but it's not implemented as a shortcut for find().steam() but as a javax.persistence.Query.getResultStream(). So maybe this needs to be rephrased (and add some notes about the use of @Transactionnal if the stream performs additional queries).

- support readOnly transactions to not inccurs too much performance penalties when using it

Regards,

Loïc

[Stephane Epardaud]

On Mon, 15 Jul 2019 at 12:17, Loïc MATHIEU <loik...@gmail.com> wrote:

- document it, in the Javadoc at least because it incorrectly states "This method is a shortcut for <code>find(query, sort, params).stream()</code>." but it's not implemented as a shortcut for find().steam() but as a javax.persistence.Query.getResultStream(). So maybe this needs to be rephrased (and add some notes about the use of @Transactionnal if the stream performs additional queries).

but find() returns a PanacheQuery whose stream() method does have this behaviour. The implementation does call find().stream() which is right, isn't it?

I'm all for mentioning this in the docs, though, but I think the implementation is correct and it is indeed a shortcut like it says.

[Sanne Grinovero]

On Mon, 15 Jul 2019 at 11:36, Stephane Epardaud
<stephane...@gmail.com> wrote:
>
>
>
> On Mon, 15 Jul 2019 at 12:17, Loïc MATHIEU <loik...@gmail.com> wrote:
>>
>> - document it, in the Javadoc at least because it incorrectly states "This method is a shortcut for <code>find(query, sort, params).stream()</code>." but it's not implemented as a shortcut for find().steam() but as a javax.persistence.Query.getResultStream(). So maybe this needs to be rephrased (and add some notes about the use of @Transactionnal if the stream performs additional queries).
>
>
> but find() returns a PanacheQuery whose stream() method does have this behaviour. The implementation does call find().stream() which is right, isn't it?

I don't think it's right to iterate over the stream outside of the
scope of a transaction, that's very likely not going to behave like
the user expects. We should A) enforce the transaction usage, B)
provide a nice way for read-only transactions like Loïc suggested.

Incidentally, while I agree on the need to have read-only transactions
I wouldn't expect there to be a significant differenec in performance;
it would be nice to couple this to set the Hibernate Session into
read-only too though, there are some performance benefits with that as
we can skip dirty checking operations.

Stef, why is Panache allowing any read out of transaction scope? That
all sounds suspicious to me.

Thanks

[Emmanuel Bernard]

On Mon, Jul 15, 2019 at 12:43 PM Sanne Grinovero <sa...@hibernate.org> wrote:

> Stef, why is Panache allowing any read out of transaction scope? That
> all sounds suspicious to me.

That's just JPA semantic to allow reads (at least lockMode.NONE ones)
outside transactions, no?

[Loïc MATHIEU]

@Stephane Epardaud yes, indeed, it's implemented as find().stream(), so it's implemented as PanacheQuery.stream(). 

It can be missguided as some may think that as it's implemented as list().stream() and it was my first guess, then I check the code and the JavaDoc and I don't know why I think there was a mistake in it ;)

As provided read out of transaction, it's not only JPA it's almost all frameworks that I knows that provides this kind of convenience. I'm used to Spring Data and MyBatis, both works without explicit transaction support.

So, I can send a PR with a small NOTE to the Panache guide with the need to add an @Transactional annotation when using the stream method to keep the DB cursor open until the end of the stream.

For preventing the user to use stream operator withour explicit transaction support, I'm not sure it's really necessary, but I can open an issue to implement it later.

For readOnly transactions, I think it's definilty worth openning an issue to be able to set it:

- Globally at Hibernate or TM level

- Per transaction in the new @TransactionConfiguration annotation.

=> I can open this PR but not working on it this week as I have tons of other thinks to do ...

Regards,

Loïc

[Stephane Epardaud]

Well, I think the interesting question is why are you reading it outside of the transaction? Who closed your transaction before you accessed the stream?

--

Stéphane Épardaud

[Loïc MATHIEU]

Well, I aways do the most simple thing possible ... and maybe I have bad habbits but in a lot of frameworks I can retrieved data from a database without taking care of the transaction (I can do it in Spring Data, MyBatis, plain JDBC, ...).

And with Panache I can do it unless I use the stream() operation.

Transactions are usually associated with database changes (even on Wikipedia: https://en.wikipedia.org/wiki/Database_transaction) but I know that it's more than this (consistent reads, ...)

Maybe it's just a bad habbit, I assume the frameworks handle automatically the transaction for me (or the database driver), but as a lot of developers, I'm lazy, so if I can avoid specifying the annotation, I dont specify it !

[Stephane Epardaud]

Oh, sorry, so it's not that you closed your transaction, it's that you never opened it in the first place. OK fine. Now I get it. Well, yeah, it's a bit confusing then that you can get the whole list without a transaction but not stream it. I could throw an exception in stream() similar to what we throw in mutating EntityManager operations to tell you to add `@Transactional` but it's still a trap.

--

Stéphane Épardaud

[Loïc MATHIEU]

Hi,

So, do we create an "action plan" for this ?

1. Add to the guide and the JavaDoc some warnings about it ? Something like the following :
"If you use one of the Panache stream() operators, and access to the database during one of it's processing step, you must do this inside a transaction (so using the `@Transactional` annotation to the method)."

2. As Stéphane suggested, generate an exception if the stream operation is used outside of a transaction (I vote no for this one as it may works so it's not mandatory)

3. Implements "read only" transactions, as I understant, this needs to be propagated to the underlying Hibernate session (but is the session tied to the transaction in Quarkus ?)

I can open related issues if needed, I can easily provides a PR for 1) but I don't know how to implements 3) so maybe someone else can do it.

Regards,

Loïc

[Sanne Grinovero]

On Wed, 17 Jul 2019 at 09:59, Loïc MATHIEU <loik...@gmail.com> wrote:
>
> Hi,
>
> So, do we create an "action plan" for this ?

+1

> 1. Add to the guide and the JavaDoc some warnings about it ? Something like the following :
> "If you use one of the Panache stream() operators, and access to the database during one of it's processing step, you must do this inside a transaction (so using the `@Transactional` annotation to the method)."

I'd also point out that the stream must be closed within the same
transaction (see below..)

> 2. As Stéphane suggested, generate an exception if the stream operation is used outside of a transaction (I vote no for this one as it may works so it's not mandatory)

Careful, it is mandatory at least for some databases: cursors are
(typically) destroyed at the end of any transaction scope. And when
you don't explicitly control a transaction, such transaction scope is
automatically created to span the single statement, so the cursor
would be destroyed right away: the effect is exactly the exception you
reported.

See e.g. PostgreSQL:
- https://jdbc.postgresql.org//documentation/head/query.html#query-with-cursor

It *might* appear to work when we fetch all results in the first
batch; but that's not something we should force right? I'd rather have
people demand that explicitly by using list().stream() (the list()
obviously requests to fetch them all).

> 3. Implements "read only" transactions, as I understant, this needs to be propagated to the underlying Hibernate session (but is the session tied to the transaction in Quarkus ?)

I don't think it is yet, but we could read the transaction hints to
newly opened Hibernate sessions.

>
> I can open related issues if needed, I can easily provides a PR for 1) but I don't know how to implements 3) so maybe someone else can do it.

Many thanks for organizing this! If you could split this in issues,
that would be great.

Sanne

> --
> You received this message because you are subscribed to the Google Groups "Quarkus Development mailing list" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to quarkus-dev...@googlegroups.com.
> Visit this group at https://groups.google.com/group/quarkus-dev.

> To view this discussion on the web visit https://groups.google.com/d/msgid/quarkus-dev/CAJLxjVFrViDfcYYa%2Btj449%2BrzNqDG5-WX_V8ExuWqSD2rJMqPw%40mail.gmail.com.

[Loïc MATHIEU]

Three issues created:

- Document the Panache stream() operations for the need of transaction: https://github.com/quarkusio/quarkus/issues/3256

    => I will propose a PR for this one

- Offers a way to configure a transaction to be readOnly: https://github.com/quarkusio/quarkus/issues/3257

- Investiguate if the Panache stream() methods should foce the usage of transactions: https://github.com/quarkusio/quarkus/issues/3258

Regards,

Loïc

[Sanne Grinovero]

Perfect, many thanks!

> To view this discussion on the web visit https://groups.google.com/d/msgid/quarkus-dev/CAJLxjVHS15E9OP-72GDKRiKbNy_-926-3FC959BNbioNTjcL5w%40mail.gmail.com.