Avoid storing default user email if not fetching codes - #705
Avoid storing default user email if not fetching codes#705edan-bainglass wants to merge 1 commit into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #705 +/- ##
==========================================
- Coverage 84.29% 84.27% -0.02%
==========================================
Files 18 18
Lines 3584 3587 +3
==========================================
+ Hits 3021 3023 +2
- Misses 563 564 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| ) | ||
|
|
||
| self._default_user_email = orm.User.collection.get_default().email | ||
| if fetch_codes: |
There was a problem hiding this comment.
Could you explain the logic? Why only store the _default_user_email conditionally? Is there any side effect to always storing_default_user_email?
There was a problem hiding this comment.
The side effect was that an error was raised by trying to access the User table. I suspect this was being accessed by a thread other than the main one, which should not happen.
There was a problem hiding this comment.
But we're not holding a pre-existing connection here, so even if there was another thread accessing this data it should not be a problem I think.
There was a problem hiding this comment.
Hmm, unless the get_default method is cached. In that case a better solution is to use a lock here imo.
There was a problem hiding this comment.
@property
def default_user(self) -> Optional['User']:
"""Return the default user for the profile, if it has been created.
This is cached, since it is a frequently used operation, for creating other entities.
"""Cached 🙂
There was a problem hiding this comment.
Yeah, now I vaguely remember that we had the same issue in QeApp with this method, might be good to how it is solved there.
There was a problem hiding this comment.
That's all discussed in the referenced PR - #543. But I don't think the ComputationalResourcesWidget was run on a thread there. Not sure. But yeah, good to have another look, when time permits 😅
There was a problem hiding this comment.
But I don't think the ComputationalResourcesWidget was run on a thread there.
I don't think you can generally guarantee that another thread will not run at the same time --- AWB is a library so we don't control how it is used. I agree it would be good to ultimately resolve this one way or the other. (but also I am sure there are many other issues like this in other widgets unfortunately.
There was a problem hiding this comment.
I think a general solution was already proposed. Stop using threads - implement and use a proper REST API.
|
The issue does not appear to be a problem. I suspect it was an temporary error in implementation that is no longer in place. Closing for now. |
3 participants
In #543, we opted to replacing direct default user fetching with fetching via the user's email, to avoid thread issues.
Since, #646 made code fetching optional in
ComputationalResourcesWidget. However, the storing of the default user's email was no guarded. This PR remedies this.#646 was introduced, as at least the in the QE app, which has moved towards MVC design, now handles its own code fetching.
However, it appears that even the storing of the default user's email introduces a threading issue (see aiidalab/aiidalab-qe#1391).
Note that this was discovered during development, in which I am threading the plugin fetching mechanism. Not yet clear where and how this leads to the bug. Investigating...
In any case, this PR simply completes the work of #646