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

fix(vertx): deploy verticles with async decorated blocking handlers #930

Merged
merged 2 commits into from
May 12, 2022

Conversation

andrewazores
Copy link
Member

Fixes #929

  • tmp: add intentionally blocking API handler for testing
  • fix(vertx): deploy verticles with async decorated blocking handlers

The first commit in this series is just for testing purposes. It adds an API handler at "/api/beta/block" that sleeps the current thread for 5 seconds, then returns a "hello" string response.

This can be used to test by doing:

https :8181/api/beta/block

in one terminal. In a second terminal, before that first request completes,

https :8181/health

In current main, the /health request will not return until after the /block request returns. After the fix in this PR is applied these requests are properly handled concurrently again.

@andrewazores andrewazores self-assigned this May 12, 2022
ebaron
ebaron previously approved these changes May 12, 2022
Copy link
Member

@ebaron ebaron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works as described for me. With just the block handler, the health check is blocked. With the fix, the health check doesn't block.

jan-law
jan-law previously approved these changes May 12, 2022
Copy link
Contributor

@jan-law jan-law left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works well for me

@andrewazores andrewazores dismissed stale reviews from jan-law and ebaron via d9726a3 May 12, 2022 20:57
@andrewazores
Copy link
Member Author

Just replaced S.o.println with logger.info and removed the BlockingHandler manual testing API endpoint. No other changes on that force push.

Copy link
Contributor

@hareetd hareetd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Web-client is working well too with the changes.

@andrewazores andrewazores merged commit 0f13392 into cryostatio:main May 12, 2022
@andrewazores andrewazores deleted the blocking-requests branch May 12, 2022 21:11
mergify bot pushed a commit that referenced this pull request May 12, 2022
…930)

* fix(vertx): deploy verticles with async decorated blocking handlers

* replace println with logger

(cherry picked from commit 0f13392)
andrewazores added a commit that referenced this pull request May 12, 2022
…930) (#934)

* fix(vertx): deploy verticles with async decorated blocking handlers

* replace println with logger

(cherry picked from commit 0f13392)

Co-authored-by: Andrew Azores <aazores@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Task] Concurrent API Requests can become blocked
4 participants