Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Remove thread-abort from corelib#10648

Closed
danmoseley wants to merge 1 commit intodotnet:masterfrom
danmoseley:threadabort
Closed

Remove thread-abort from corelib#10648
danmoseley wants to merge 1 commit intodotnet:masterfrom
danmoseley:threadabort

Conversation

@danmoseley
Copy link
Copy Markdown
Member

Type is unnecessarily duplicated with CoreFX; I was going to move it down, then realized I can remove all usages from corelib instead. This also gets several files closer to their CoreRT counterparts so that they can be moved to shared in a subseqent change.

I did not attempt to remove thread abort throughout the VM, EE etc because that's a much more substantial change and some places I'm not certain of.

@jkotas is it correct that the managed code can be oblivious to the remaining native code? For example, if a funceval is aborted, is there any way an exception can reach managed code that's a thread abort? Or is this change safe.

@alexperovich

@danmoseley
Copy link
Copy Markdown
Member Author

@stephentoub

/// <summary>
/// The ThreadPool calls this if a ThreadAbortException is thrown while trying to execute this workitem. This may occur
/// before Task would otherwise be able to observe it.
/// </summary>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We have backed away from removing this earlier to make this easier to share with Mono:
#9234 (comment)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(And it is marginal goodness for the funceval case as well.)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

How do you see enabling sharing with CoreRT? For Task.cs (eg) it has all the TAE code commented out. I was hoping to make this shareable with this change.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should add it back to CoreRT.

@cshung is adding FuncEval to CoreRT. I expect that we will want to make a basic thread abort work well there, and the thread abort handling in tasks is needed for it.

I do not expect that we are going to harden it under stress. If you enter bad expression in a watch window that times out evaluating, there will be some small chance that your program state will get corrupted. It is not really different from where we are with netfx today. However, we will want fix common cases where it is 100% chance. The thread abort handling in tasks is an example of that.

@jkotas
Copy link
Copy Markdown
Member

jkotas commented Apr 2, 2017

is it correct that the managed code can be oblivious to the remaining native code

As you can see from the red CI, it is not the case.

For example, if a funceval is aborted

Given the thread abort is used internally by funceval and this use can be seen by external external code in some cases, I think it may be best to move the ThreadAbortException to CoreLib.

@danmoseley
Copy link
Copy Markdown
Member Author

I'll abandon this and at some point someone can move TAE per #16229

@danmoseley danmoseley closed this Apr 3, 2017
@danmoseley danmoseley deleted the threadabort branch April 3, 2017 20:03
@karelz karelz modified the milestone: 2.0.0 Aug 28, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants