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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix CA transactions warnings #2347

Merged
merged 3 commits into from Apr 29, 2017
Merged

Fix CA transactions warnings #2347

merged 3 commits into from Apr 29, 2017

Conversation

tiennou
Copy link
Member

@tiennou tiennou commented Apr 19, 2017

This should fix the CA transaction warnings we have (at least the ones I saw).

I went pretty heavy-handed on the proxy thing, but I didn't want to make all QSTask's accessors to become main-thready-asyncy...

I'll check that I didn't forget some of the keypaths though (I haven't checked everything, and I don't think we have an easy indeterminate progress one anywhere).

It seems to also fix the flicker we had, so 馃憤.

tiennou added 2 commits Apr 19, 2017
Those notifications come for "some thread", and this is a main-thread-only progress indicator
@tiennou
Copy link
Member Author

@tiennou tiennou commented Apr 19, 2017

Check completed. I manually switched scanTask and it didn't fail.

@skurfer
Copy link
Member

@skurfer skurfer commented Apr 20, 2017

I assume we can close #2329?

@tiennou
Copy link
Member Author

@tiennou tiennou commented Apr 20, 2017

Ref #2275

@tiennou
Copy link
Member Author

@tiennou tiennou commented Apr 20, 2017

Sorry, that one slipped my radar. I'm not sure actually, #2329 does (IMHO) weird things[1] to tasks, and the 2 other changes are unrelated, so they might be needed. My run was mostly "background", so I might not have got some of the warnings that we can get when eg. actions are executed.

[1] I don't see how changing to a sync block can fix anything. At worst, you might improve the deadlock rate because now whatever caused the task view to update (like a notification) will be waiting on the main thread.

@skurfer
Copy link
Member

@skurfer skurfer commented Apr 20, 2017

I just did like it said and set CA_DEBUG_TRANSACTION, but I don鈥檛 remember exactly what I found or why I did things that way. It was a loooong time ago.

@skurfer
Copy link
Member

@skurfer skurfer commented Apr 20, 2017

Looking back, we do need those changes, or something like them, no? Those things shouldn鈥檛 be allowed to happen on a background thread.

@tiennou
Copy link
Member Author

@tiennou tiennou commented Apr 20, 2017

True, but if it's called on the main thread, it's the fault of whatever called them not on the main thread (that's how I consider everything in QSInterface). So IMHO those are not correct fixes per se.

@skurfer skurfer merged commit f7af8f7 into master Apr 29, 2017
2 checks passed
@skurfer skurfer deleted the fix/ca-transactions branch Apr 29, 2017
skurfer added a commit that referenced this issue Apr 29, 2017
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