Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reject stream executions if not executed within transaction [DATAJPA-1023] #1369

Closed
spring-projects-issues opened this issue Dec 13, 2016 · 6 comments
Assignees
Labels
type: bug
Milestone

Comments

@spring-projects-issues
Copy link

@spring-projects-issues spring-projects-issues commented Dec 13, 2016

Maarten Hus opened DATAJPA-1023 and commented

I'm using Stream's to loop through each element in a Repository in an @Scheduled method. After running 200 times I get the following error:

2016-12-13 09:40:39.159  WARN 51004 --- [pool-2-thread-1] o.h.engine.jdbc.spi.SqlExceptionHelper   : SQL Error: 0, SQLState: null
2016-12-13 09:40:39.159 ERROR 51004 --- [pool-2-thread-1] o.h.engine.jdbc.spi.SqlExceptionHelper   : [pool-2-thread-1] Timeout: Pool empty. Unable to fetch a connection in 2 seconds, none available[size:200; busy:200; idle:0; lastwait:2000].

The Repository is defined as such:

public interface PushNotificationRepository extends JpaRepository<PushNotification, Long> {
    @Query(value = "select p from PushNotification p")
    Stream<PushNotification> findAllAndStream();
}

The Service is defined like this:

@Service
@Transactional()
public class PushNotificationFlusherService {
    private static final Logger LOGGER = LoggerFactory.getLogger(PushNotificationFlusherService.class);

    private final PushNotificationRepository pushNotificationRepository;

    @Autowired
    public PushNotificationFlusherService(PushNotificationRepository pushNotificationRepository) {
        this.pushNotificationRepository = pushNotificationRepository;
    }

    @Scheduled(fixedDelay = 5000)
    protected void flushPushNotificationQueue() {
        LOGGER.info("Flushing push notifications");

        try (Stream<PushNotification> pushNotifications = pushNotificationRepository.findAllAndStream()) {
            pushNotifications.forEach((PushNotification pushNotification) -> {
               // Send notification 
            });
        }

        LOGGER.info("Flushed push notifications");
    }

I've tried using @Transactional( readOnly=true) but it does not make a difference.


Affects: 1.10.5 (Hopper SR5)

Issue Links:

  • DATACMNS-959 Register repository interceptor to allow detecting a surrounding transaction
    ("depends on")
  • DATAJPA-989 CrudRepository; return Stream<T>; The object is already closed [90007-192]; could not advance using next()
    ("is duplicated by")

Backported to: 1.11 RC1 (Ingalls)

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Dec 13, 2016

Oliver Drotbohm commented

Do you run transactions in AspectJ mode? If not, adding @Transactional doesn't have any effect for protected methods. Making the method public (and maybe the entire service class package protected to avoid general visibility) should do the trick here

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Dec 13, 2016

Maarten Hus commented

Thank you @OlivierGierke, making the method public did the trick.

Thanks again for your lighting fast response (y)

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Dec 13, 2016

Oliver Drotbohm commented

No worries, glad we got it working for you. We're currently investigating to even throw an exception in case we detect the transactions not being set up correctly when executing a method using Stream as a return type. I'll keep this open and potentially commit that fix against this ticket here

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Dec 14, 2016

Oliver Drotbohm commented

This is now in place. A stream execution now checks whether there has been a transaction in place before the repository was invoked. If that's not the case, we reject the execution and give an indication why this is happening. This should give you a better idea of why things are not working in case you accidentally run into some misconfiguration like the one shown above

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Jun 1, 2017

Neale Upstone commented

It's good to have this in place.

Oliver. Can you clarify the original behaviour and limitations of the new behaviour (I thought it's best to add it here as this is where we end up from the release notes).

The original behaviour seems to be a connection leakage either in Data JPA or in the underlying JPA provider's streaming support.

  • Was there not an option to release the connection once the end of the result set was reached?
  • Are we limited in having to wrap in a transaction or can we also add the OpenEntityManagerInViewFilter to allow us to stream all the way to the HttpResponse output stream?
    • Looking at OpenEntityManagerInViewFilter it appears that async is handled too, so perhaps this is okay and perhaps a good example is all that's needed

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Sep 5, 2017

Alexander Simeonov commented

@OlivierGierke I believe that the advantage of Streams is processing very large data sets, or am I wrong in this assertion? If you are processing a lot of data @Transactinal is not really an option at all. I have experiences this in production and had to revert to pagination on Lists instead of Streams. When you try to wrap a very long running Stream over a huge set of data in @Transactional what ends up happening is progressive slowdown of you database operations (performance) over time, and huge space usage for Transaction caching/checkpoints on the database end which eventually could blow things up, it suffices to say that this behavior and performance degradation is just not desirable at all. This is all expected as we are doing a huge Transaction on this large data set, but then Streams are really not an option at all for such a large amount of data. Switching to List Pages outside of a Transaction from @Transaction with Streams literally improved performance of my app by a couple of orders of magnitude. Please reopen this issue and make Streams work without @Transaction as they are simply not very useful otherwise and their performance on large dataset is simply unacceptable. Unless of course large data sets is not their intended use case

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug
Projects
None yet
Development

No branches or pull requests

2 participants