search

snowdrop / vertx-spring-boot

Vert.x Spring Boot integration brings different Vert.x components to Spring Boot ecosystem
Apache License 2.0
137 stars 41 forks source link

Blocking calls in kafta tests #129

Open Arooba-git opened 1 year ago

Arooba-git commented 1 year ago

HI! 🙂

Thank you for this project.

As we were going through the source , we found these blocking calls in AbstractIT class of the Kafka.it tests:

vsb2-blocking

This method is called in another method where a timed await is called immediately after calling the above (sendToTopic) method:

Screen Shot 2023-07-24 at 6 19 48 PM

Which got us thinking , if we are waiting anyway, then why use .block operator and block the event loop.. Would it not be just fine to subscribe? 🤔 We tried the test cases with subscribe() and they still passed..

We also evaluated the performance difference when using block vs subscribe (in terms of thread count and heap usage), and the subscribe version seem to outperform in both.

I know this is just test code but considering many users are and will be using it for their project, we might as well set good example by avoiding blocking calls wherever possible in a reactive chain/event loop.

We created a PR with the change in case you deem it fit: https://github.com/snowdrop/vertx-spring-boot/pull/130 :)

cescoffier commented 1 year ago

I don't think the test runs on the event loop, so blocking is acceptable.

That being said, in this case, block() may not be necessary. In this case, it waits for the acknowledgment from the broker. However, if the send fails, it would throw an exception, giving more information about the problem than just a timeout in the await call.

Another aspect to consider is that, anyway, the send method may block (batching on the client side), even if used without block().

Arooba-git commented 1 year ago

@cescoffier Thank you for the detailed response!

I don't think the test runs on the event loop, so blocking is acceptable.

Hmmm tbh I'm not an expert on reactive programming, but this .block() in test was also detected by BlockHound, so we guessed it implies some main thread (in this case event loop) is indeed being blocked 🤔

However, if the send fails, it would throw an exception, giving more information about the problem than just a timeout in the await call.

Makes sense :). though is there not a more reactive approach of handling errors in such scenarios? 🤔

cescoffier commented 1 year ago

Do you have the name of the thread running the test?

Anyway, if that test must not be blocked, I don't believe sending like this should be allowed (probably not detected, but it can block).

Arooba-git commented 1 year ago

Do you have the name of the thread running the test?

hm, I would have to rerun it again with the profiler, let me get back to you once I have it. 👍