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

traceStartTask / traceEndTask: use 'use' block #1408

Merged
merged 4 commits into from
Oct 30, 2016

Conversation

0x53A
Copy link
Contributor

@0x53A 0x53A commented Oct 25, 2016

So it seems in my previous PR (#1379) I missed a few cases ...

The whole try / finally stuff just adds noise (in my opinion), what do you think about this IDisposable approach?

If you like it, I will go through all files (a full text search for traceStartTask found 114 lines in 59 files, so totally manageable).

@0x53A
Copy link
Contributor Author

0x53A commented Oct 25, 2016

In many cases it explicitly either closes the task or throws, so maybe this was originally on purpose?

Another approach could be to walk the stack on close, and close everything until the specified tag.
(e.g. if I call open "A" -> open "B" -> close "A", then B is implicitly closed.)

Then I could revert my previous pr and it would "just work".

@forki
Copy link
Member

forki commented Oct 26, 2016

I like the idea of the use thingy. Can you add that everywhere?

@0x53A 0x53A force-pushed the trace-start-end-task branch from 7992dff to c6d091f Compare October 26, 2016 18:46
0x53A added 2 commits October 26, 2016 20:48
… that it is always closed, even if the task throws an exception."

This reverts commit 88fde73.
@0x53A 0x53A force-pushed the trace-start-end-task branch from c6d091f to a638345 Compare October 26, 2016 18:57
@0x53A 0x53A force-pushed the trace-start-end-task branch from a638345 to ccda0fd Compare October 26, 2016 19:13
@0x53A
Copy link
Contributor Author

0x53A commented Oct 26, 2016

Ok that should be everything this time

@forki
Copy link
Member

forki commented Oct 30, 2016

thanks so much

@forki forki merged commit ccda0fd into fsprojects:master Oct 30, 2016
@0x53A 0x53A deleted the trace-start-end-task branch April 16, 2017 14:41
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.

2 participants