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

added wait timeout to server start #918

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gittiver
Copy link
Member

No description provided.

@gittiver gittiver linked an issue Oct 14, 2024 that may be closed by this pull request
@fliiiix
Copy link
Contributor

fliiiix commented Oct 15, 2024

I quickly tested it and it worked fine for me 🥳 The only downside i see is that we still lose the information why it failed

@gittiver
Copy link
Member Author

The only downside i see is that we still lose the information why it failed

Isn't that logged?

@fliiiix
Copy link
Contributor

fliiiix commented Oct 15, 2024

hmm, I see, the timeout can have different reasons and in addition the Acceptor and endpoint are created in the constructor of the server. Would make this ticket bigger.

@fliiiix
Copy link
Contributor

fliiiix commented Oct 18, 2024

Did you written your answer on purpose as edit on my comment? 😆

I mean i think this fix already brings value the way it is now but to fully fix it it probably should log the errors that happen.

@gittiver
Copy link
Member Author

Did you written your answer on purpose as edit on my comment? 😆
No, I was in a hurry.

I mean i think this fix already brings value the way it is now but to fully fix it it probably should log the errors that happen.
I agree, I was thinking about a separate PR for that to keep changes in this one simple.

@gittiver gittiver marked this pull request as draft October 18, 2024 14:18
@gittiver gittiver self-assigned this Oct 21, 2024
@gittiver gittiver force-pushed the 885-there-are-no-exceptions-with-run_async branch from f872423 to 21ee6b7 Compare November 4, 2024 21:07
@gittiver gittiver force-pushed the 885-there-are-no-exceptions-with-run_async branch from 21ee6b7 to d566cb9 Compare November 25, 2024 12:59
@gittiver gittiver marked this pull request as ready for review November 25, 2024 20:48
@gittiver
Copy link
Member Author

Invalid address strings are now logged, so mission complete. :)

include/crow/app.h Outdated Show resolved Hide resolved
@fliiiix
Copy link
Contributor

fliiiix commented Nov 26, 2024

I found another case 🙈 where if the port is already used we also just see a timeout:

(2024-11-26 08:10:18) [INFO    ] Before wait
(2024-11-26 08:10:21) [INFO    ] After wait
(2024-11-26 08:10:21) [INFO    ] Timeout

But im not sure if we can check for that, and this version is already way better than before 🥳

@gittiver
Copy link
Member Author

I found another case 🙈 where if the port is already used we also just see a timeout:

(2024-11-26 08:10:18) [INFO    ] Before wait
(2024-11-26 08:10:21) [INFO    ] After wait
(2024-11-26 08:10:21) [INFO    ] Timeout

But im not sure if we can check for that, and this version is already way better than before 🥳

This has to be handled inside of the server, I would propose to handle in a different issue - or maybe there are already some for this.

… logged now, server runs into timeout in that case

added testcase for invalid address
@gittiver gittiver force-pushed the 885-there-are-no-exceptions-with-run_async branch from f66f065 to e31486d Compare November 26, 2024 09:35
Copy link
Contributor

@fliiiix fliiiix left a comment

Choose a reason for hiding this comment

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

nice 🥳

Copy link
Contributor

@fliiiix fliiiix left a comment

Choose a reason for hiding this comment

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

Ran some manual tests against this latest version which worked fine for me

[100%] Built target radiopi
(2024-11-26 11:49:19) [INFO    ] Playing some music
(2024-11-26 11:49:19) [ERROR   ] Invalid argument - Can not create valid ip address from string: "0.0.0.a"

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

Successfully merging this pull request may close these issues.

There are no exceptions with run_async()
2 participants