fix(developer): warn only on race when destroying TAppSourceHttpResponder#16140
fix(developer): warn only on race when destroying TAppSourceHttpResponder#16140mcdurdin wants to merge 1 commit into
Conversation
…nder I uncovered one race, documented in the source. Not trying to resolve that race at this time (it is not consequential). There is a second condition which is unclear -- and may have other consequences. So report a warning to Sentry when this arises, but do not crash out on the user. Fixes: #11916 Test-bot: skip
User Test ResultsTest specification and instructions User tests are not required |
| procedure TframeTextEditor.TntFormDestroy(Sender: TObject); | ||
| begin | ||
| inherited; | ||
| if FFileName <> '' then | ||
| modWebHttpServer.AppSource.UnregisterSource(FFileName); | ||
|
|
||
| FreeAndNil(FCharFont); | ||
| FreeAndNil(FCodeFont); | ||
| end; |
There was a problem hiding this comment.
From devin.ai:
Text editor does not guard UnregisterSource with IsSourceRegistered unlike touch layout builder
In TntFormDestroy (UframeTextEditor.pas:1153-1154), the text editor calls UnregisterSource(FFileName) directly, protected only by a FFileName <> '' check. By contrast, the touch layout builder at UframeTouchLayoutBuilder.pas:238-241 calls IsSourceRegistered before UnregisterSource. If the race condition described in the new destructor comments (lines 83-90 of the AppSource file) triggers in reverse — where the source is already unregistered before destruction — the text editor's UnregisterSource will hit the Assert(False, ...) at Keyman.Developer.System.HttpServer.AppSource.pas:366. This is a pre-existing inconsistency, not introduced by this PR, but the PR's new comments explicitly document that such races exist, making it worth noting.
| // Note: Unlike regular functions, Assert has short-circuit evaluation | ||
| // intrinsics on the first param which makes it safe to dereference T[0] in | ||
| // the second parameter. |
There was a problem hiding this comment.
This comment is now outdated and should be removed or updated
2 participants
I uncovered one race, documented in the source. Not trying to resolve that race at this time (it is not consequential). There is a second condition which is unclear -- and may have other consequences. So report a warning to Sentry when this arises, but do not crash out on the user.
Fixes: #11916
Test-bot: skip