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

refactor: make shell.ShowItemInFolder asynchronous #17121

Merged
merged 4 commits into from Feb 27, 2019

Conversation

@codebytere
Copy link
Member

commented Feb 25, 2019

Description of Change

BREAKING CHANGE: change return type of ShowItemInFolder to void from bool and offload task to new thread.

cc @nornagon @deepak1556

Checklist

Release Notes

Notes: Made ShowItemInFolder asynchronous with no return value.

@codebytere codebytere requested a review from as a code owner Feb 25, 2019

@codebytere codebytere changed the title fix: add scoped_blocking_calls to platform_util_win chore: add scoped_blocking_calls to platform_util_win Feb 25, 2019

@deepak1556
Copy link
Member

left a comment

There should be another sweep in the codebase to mark other blocking call sites, also whats the benefit of these two apis returning synchronous boolean results ? If it can be deprecated for async version, we could offload the task to a different thread.

@zcbenz
Copy link
Member

left a comment

The windows CI fails with:

[4264:0225/130237.106:FATAL:thread_restrictions.cc(34)] Check failed: !g_blocking_disallowed.Get().Get(). Function marked as blocking was called from a scope that disallows blocking! If this task is running inside the TaskScheduler, it needs to have MayBlock() in its TaskTraits. Otherwise, consider making this blocking work asynchronous or, as a last resort, you may use ScopedAllowBlocking (see its documentation for best practices).
Backtrace:
	base::debug::CollectStackTrace [0x0348BBD3+19]
	logging::LogMessage::~LogMessage [0x0347EEE2+98]
	base::ScopedBlockingCall::ScopedBlockingCall [0x03543EFC+204]
	platform_util::OpenExternal [0x055956B8+33]

It seems that blocking calls are disallowed somewhere up in the call stack.

@deepak1556

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

Oops missed that, yeah the main thread disallows blocking calls. We can't add base::ScopedAllowBlockng without patch to upstream. Best way forward is to move these tasks to a different thread, then again its not possible with synchronous api versions.

@deepak1556

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

also whats the benefit of these two apis returning synchronous boolean results ? If it can be deprecated for async version, we could offload the task to a different thread.

@zcbenz what are your thoughts on this ?

@zcbenz

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

Deprecating sync versions looks good to me.

@codebytere

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2019

@zcbenz @deepak1556 sounds good to me! I'll start working on that :)

@codebytere codebytere force-pushed the add-scoped-blocking branch from 73fbd4e to 961cf75 Feb 26, 2019

@codebytere codebytere requested a review from as a code owner Feb 26, 2019

@codebytere codebytere force-pushed the add-scoped-blocking branch from 961cf75 to a441417 Feb 26, 2019

@codebytere codebytere changed the title chore: add scoped_blocking_calls to platform_util_win refactor: make ShowItemInFolder asynchronous Feb 26, 2019

@codebytere codebytere changed the title refactor: make ShowItemInFolder asynchronous refactor: make shell.ShowItemInFolder asynchronous Feb 26, 2019

@codebytere codebytere force-pushed the add-scoped-blocking branch from a441417 to aecfbe7 Feb 26, 2019

@codebytere codebytere force-pushed the add-scoped-blocking branch from aecfbe7 to 17c26a2 Feb 26, 2019

@electron-cation electron-cation bot removed the new-pr 🌱 label Feb 26, 2019

codebytere added some commits Feb 26, 2019

@zcbenz

zcbenz approved these changes Feb 26, 2019

Copy link
Member

left a comment

We should probably add this to breaking-changes.md, the code looks good to me.

@nornagon

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

NB. you can also write the line "BREAKING CHANGE" in the PR description to have it listed in that section of the release notes.

@codebytere codebytere merged commit 5ecda17 into master Feb 27, 2019

14 of 15 checks passed

Artifact Comparison Changes Detected
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-debug AppVeyor build succeeded
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-debug AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
build-linux Workflow: build-linux
Details
build-mac Workflow: build-mac
Details
electron-arm-testing Build #20190226.19 succeeded
Details
electron-arm64-testing Build #20190226.19 succeeded
Details
lint Workflow: lint
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

commented Feb 27, 2019

Release Notes Persisted

Made ShowItemInFolder asynchronous with no return value.

@codebytere codebytere deleted the add-scoped-blocking branch Feb 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.