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

Don't use generic ThreadKilled #79

Closed
wants to merge 1 commit into from

Conversation

edsko
Copy link
Contributor

@edsko edsko commented Jul 20, 2023

By using a specific exception debugging of client code becomes easier: when we see a thread killed using ThreadKilled, it could come from anywhere, but when we see a thread killed using KilledByHttp2ThreadPoolManager, it can only come from one place.

By using a specific exception debugging of client code becomes easier: when we
see a thread killed using `ThreadKilled`, it could come from anywhere, but when
we see a thread killed using `KilledByHttp2ThreadPoolManager`, it can only come
from one place.
@edsko
Copy link
Contributor Author

edsko commented Jul 20, 2023

CI timeout on Windows is I think not related to this PR?

@kazu-yamamoto kazu-yamamoto self-requested a review July 21, 2023 01:31
Copy link
Owner

@kazu-yamamoto kazu-yamamoto left a comment

Choose a reason for hiding this comment

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

Nice idea!

@kazu-yamamoto
Copy link
Owner

@edsko Don't worry about Windows stuff.
The current network package does not support Windows well.
That is, recv is blocking.
We are trying to integrate the new IO manager stuff to network now.
Until the integration, I don't care CI failures relating to Windows.

kazu-yamamoto added a commit that referenced this pull request Jul 21, 2023
@kazu-yamamoto
Copy link
Owner

Rebased and merged.
Thank you for your contribution!

@edsko
Copy link
Contributor Author

edsko commented Jul 24, 2023

@kazu-yamamoto I am working on #77 again, when I noticed

| Just ThreadKilled <- E.fromException e -> return False

Perhaps I should have changed that line as part of this PR?

@edsko edsko deleted the edsko/killThread branch July 24, 2023 10:33
@kazu-yamamoto
Copy link
Owner

Yeah. Would you send another PR to fix this?

edsko added a commit to edsko/http2 that referenced this pull request Jul 25, 2023
@edsko
Copy link
Contributor Author

edsko commented Jul 25, 2023

Yeah. Would you send another PR to fix this?

Yup, #81.

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.

None yet

2 participants