Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Fire diagnostic source events from IHostBuilder.Build #53757
Fire diagnostic source events from IHostBuilder.Build #53757
Changes from 13 commits
42b36a1
ec7831b
d6cded5
1c7b509
d2b5678
99d2a77
250c87d
08729fe
e592f10
e56785a
19e412e
12ae4e9
ad5c27e
7b01d0b
f8f2a69
5625ba5
315575d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use a different message here - specifically call out that it timed out. That way we can tell the difference between the entrypoint returning gracefully vs. timing out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want the timeout to be an implementation detail. I'm also thinking this could also return null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Who do you expect to see the message? If the message is primarily shown to people who are trying to resolve the issue (or users will relay it to someone else who resolves the issue) then it might help to make it more descriptive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to check the callers but its possible null is a better return than throwing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is where we wire up to the new event to intercept calls to build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The prize for dirty hackery this week : D
Is it important to guarantee that the thread really does stop? It would be easy enough for an app to throw a try/catch around the part of their code where this gets raised so that it never gets back to the entrypoint. Your tool code will be running in parallel with the user's unknown error handling app code. It doesn't seem obviously harmful to me but figured I mention it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disposal is one of the things I'm worried about and why I think throwing to stop execution makes the most sense here. The main app never gets a handle on the application here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing that stops this from happening in the throw case is the fact that the application never gets access to the IHost instance. They can't call build again on it because double building throws. Even if they catch the exception and do something else, the IHost isntance that came out of Build is never observed by the application.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I wasn't imagining they continue normally, more like they have some complex error handling code that will be running in parallel.
[Joking] What do you mean? This PR just added the official mechanism that lets everyone access IHost without the pesky inconvenience of needing Build() to return it to you ;p