mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-10-05 14:46:55 +01:00
improve knowledge article
This commit is contained in:
parent
da0f5498e5
commit
0e2a0633d2
3 changed files with 11 additions and 24 deletions
|
|
@ -27,7 +27,6 @@ codeunit 50546 "Process Driver Bad Sample"
|
||||||
begin
|
begin
|
||||||
ProcessState.SetProcessRunning(true);
|
ProcessState.SetProcessRunning(true);
|
||||||
RunSharedCode(DocumentNo);
|
RunSharedCode(DocumentNo);
|
||||||
FinishProcess(DocumentNo);
|
|
||||||
// An error above never reaches this line. The database writes roll
|
// An error above never reaches this line. The database writes roll
|
||||||
// back, the single-instance variable does not: ProcessRunning stays
|
// back, the single-instance variable does not: ProcessRunning stays
|
||||||
// true until the company is closed, so every later run in this session
|
// true until the company is closed, so every later run in this session
|
||||||
|
|
@ -38,21 +37,16 @@ codeunit 50546 "Process Driver Bad Sample"
|
||||||
local procedure RunSharedCode(DocumentNo: Code[20])
|
local procedure RunSharedCode(DocumentNo: Code[20])
|
||||||
begin
|
begin
|
||||||
end;
|
end;
|
||||||
|
|
||||||
local procedure FinishProcess(DocumentNo: Code[20])
|
|
||||||
begin
|
|
||||||
end;
|
|
||||||
}
|
}
|
||||||
|
|
||||||
// Anti-pattern 2: the binding is used correctly, but the context stays private.
|
// Anti-pattern 2: the context stays private. Flag and driver look like the
|
||||||
// The query is internal, so only the owning app can ever ask.
|
// good sample, but the query is internal, so only the owning app can ever ask.
|
||||||
codeunit 50547 "Process Ctx Bad Sample"
|
codeunit 50547 "Process Ctx Bad Sample"
|
||||||
{
|
{
|
||||||
internal procedure IsProcessRunning(): Boolean
|
internal procedure IsProcessRunning(): Boolean
|
||||||
var
|
var
|
||||||
IsRunning: Boolean;
|
IsRunning: Boolean;
|
||||||
begin
|
begin
|
||||||
IsRunning := false;
|
|
||||||
OnCheckProcessRunning(IsRunning);
|
OnCheckProcessRunning(IsRunning);
|
||||||
exit(IsRunning);
|
exit(IsRunning);
|
||||||
end;
|
end;
|
||||||
|
|
|
||||||
|
|
@ -8,7 +8,6 @@ codeunit 50540 "Process Context Good Sample"
|
||||||
var
|
var
|
||||||
IsRunning: Boolean;
|
IsRunning: Boolean;
|
||||||
begin
|
begin
|
||||||
IsRunning := false;
|
|
||||||
OnCheckProcessRunning(IsRunning);
|
OnCheckProcessRunning(IsRunning);
|
||||||
exit(IsRunning);
|
exit(IsRunning);
|
||||||
end;
|
end;
|
||||||
|
|
@ -43,11 +42,9 @@ codeunit 50542 "Process Driver Good Sample"
|
||||||
ProcessFlag: Codeunit "Process Flag Good Sample";
|
ProcessFlag: Codeunit "Process Flag Good Sample";
|
||||||
begin
|
begin
|
||||||
// A fresh instance, bound for exactly this call. If the shared code
|
// A fresh instance, bound for exactly this call. If the shared code
|
||||||
// errors, the stack unwinds, ProcessFlag leaves scope and the binding
|
// errors, the stack unwinds and takes the binding with it - nothing to reset.
|
||||||
// goes with it - no reset code, no UnbindSubscription in an error path.
|
|
||||||
BindSubscription(ProcessFlag);
|
BindSubscription(ProcessFlag);
|
||||||
RunSharedCode(DocumentNo);
|
RunSharedCode(DocumentNo);
|
||||||
FinishProcess(DocumentNo);
|
|
||||||
end;
|
end;
|
||||||
|
|
||||||
local procedure RunSharedCode(DocumentNo: Code[20])
|
local procedure RunSharedCode(DocumentNo: Code[20])
|
||||||
|
|
@ -55,10 +52,6 @@ codeunit 50542 "Process Driver Good Sample"
|
||||||
// A base application report, a posting routine, or any other object
|
// A base application report, a posting routine, or any other object
|
||||||
// that extensions hook into - including a customer's own replacement.
|
// that extensions hook into - including a customer's own replacement.
|
||||||
end;
|
end;
|
||||||
|
|
||||||
local procedure FinishProcess(DocumentNo: Code[20])
|
|
||||||
begin
|
|
||||||
end;
|
|
||||||
}
|
}
|
||||||
|
|
||||||
// An extension hooked into that shared code can now ask the question directly
|
// An extension hooked into that shared code can now ask the question directly
|
||||||
|
|
|
||||||
|
|
@ -13,15 +13,15 @@ application-area: [all]
|
||||||
|
|
||||||
## Description
|
## Description
|
||||||
|
|
||||||
An extension that drives a process over shared code — a base application report, a posting routine, a codeunit any caller may invoke — leaves every other extension hooked into that same shared code with a question the platform cannot answer: is this run part of that process, or an ordinary one? AL keeps no ambient "current process", so the driving app has to publish the context itself. The reflex answer, a `SingleInstance` codeunit holding a boolean that is set at the start of the run and cleared at the end, is unsafe in Business Central: single-instance variables are not part of the database transaction, so when the run fails the writes roll back and the flag does not. It stays `true` until the company is closed, and every later run in that session is silently treated as part of the process. A manual event binding carries the same signal safely, because the platform ties its lifetime to a variable's scope instead of to cleanup code that has to run.
|
When an extension drives a process over shared code — a base application report, a posting routine — other extensions hooked into that code cannot tell whether a run belongs to that process: AL keeps no ambient "current process", so the driving app has to publish the context itself. The reflex answer, a `SingleInstance` codeunit holding a boolean set at the start of the run and cleared at the end, is unsafe: single-instance variables are not part of the database transaction, so a failed run rolls back the writes but not the flag, which stays `true` until the company is closed and marks every later run in the session as part of the process. A manual event binding carries the same signal safely, because the platform ties its lifetime to a variable's scope instead of to cleanup code that has to run.
|
||||||
|
|
||||||
## Best Practice
|
## Best Practice
|
||||||
|
|
||||||
Publish the context as a query and let the binding itself be the state. One procedure is public; everything behind it is internal:
|
Publish the context as a query and let the binding itself be the state. One procedure is public; everything behind it is internal:
|
||||||
|
|
||||||
- A context codeunit — public, so dependent extensions can name it — exposes a public procedure such as `IsProcessRunning(): Boolean`, which raises an `[InternalEvent]` publisher taking a `var Boolean` and returns what comes back. That procedure is the entire public surface. The publisher is an internal event because only the owning app ever subscribes to it, and `local` because only this codeunit ever raises it.
|
- A public context codeunit exposes `IsProcessRunning(): Boolean`, which raises an `[InternalEvent]` publisher taking a `var Boolean` and returns what comes back — the entire public surface. The publisher is internal because only the owning app subscribes, `local` because only this codeunit raises it.
|
||||||
- A second codeunit, `Access = Internal` with `EventSubscriberInstance = Manual`, subscribes to that event and sets the boolean to `true`. It is implementation rather than API, and it keeps nothing between runs — being bound *is* the state.
|
- A second codeunit, `Access = Internal` with `EventSubscriberInstance = Manual`, subscribes to that event and sets the boolean to `true`. Internal keeps it out of the API and stops other apps binding it to forge the context; it stores nothing between runs — being bound *is* the state.
|
||||||
- The driving process calls `BindSubscription` on a variable whose scope is exactly the span it wants to claim: a local in the procedure that drives the run, or a global on an object that lives exactly as long as the run does. While that variable is alive the query answers `true`; when it leaves scope — on the normal path, or because an error unwound the call stack — the platform removes the binding and the query answers `false` again. Nothing has to be reset, so there is no cleanup path to forget and no `UnbindSubscription` to place in an error handler.
|
- The driving process calls `BindSubscription` on a variable whose scope is exactly the span it wants to claim: a local in the procedure that drives the run, or a global on an object that lives exactly as long as the run. While that variable is alive the query answers `true`; when it leaves scope — normally, or because an error unwound the call stack — the platform removes the binding and the query answers `false` again.
|
||||||
|
|
||||||
Bind a fresh instance per run rather than reusing one: the platform refuses to bind the same instance twice but accepts several instances of the same codeunit, so nesting and re-entrancy need no counter. The binding is session-scoped, so work the process starts in another session — a background session, a page background task, a job queue entry — cannot see it; pass the context explicitly there.
|
Bind a fresh instance per run rather than reusing one: the platform refuses to bind the same instance twice but accepts several instances of the same codeunit, so nesting and re-entrancy need no counter. The binding is session-scoped, so work the process starts in another session — a background session, a page background task, a job queue entry — cannot see it; pass the context explicitly there.
|
||||||
|
|
||||||
|
|
@ -29,12 +29,12 @@ See sample: `expose-process-context-via-manually-bound-flag.good.al`.
|
||||||
|
|
||||||
## Anti Pattern
|
## Anti Pattern
|
||||||
|
|
||||||
Two shapes, both of which leave other extensions unable to integrate correctly.
|
Two shapes.
|
||||||
|
|
||||||
First, the single-instance boolean. The reset at the end of the routine never runs when the process errors, so the flag survives the rollback and poisons the rest of the session. Detection: a `SingleInstance = true` codeunit with a boolean set before a process and cleared after it, read by other code to decide whether that process is running.
|
First, the single-instance boolean — the failure described above. Detection: a `SingleInstance = true` codeunit with a boolean set before a process and cleared after it, read by other code to decide whether that process is running.
|
||||||
|
|
||||||
Second, the context kept private. The driving app arranges its own marker — typically a manually bound subscriber on an event added to the shared code for its benefit alone — and offers no way to ask about it, or only an `internal` query its own module can call. Extensions hooked into the same shared code are left inferring the context from side effects, request-page values, or record state, which breaks silently the first time the process changes. Detection: a manual binding used purely as an internal run marker, with no public query procedure over it.
|
Second, the context kept private: the driving app arranges its own marker — typically a manually bound subscriber on an event added for its benefit alone — and offers no query, or only an `internal` one. Other extensions are left inferring the context from side effects, request-page values, or record state, which breaks silently the first time the process changes. Detection: a manual binding used purely as an internal run marker, with no public query procedure over it.
|
||||||
|
|
||||||
The mirror-image anti-pattern belongs to the reviewer, human or agent: reporting the `BindSubscription` in this pattern as a leaked binding because no `UnbindSubscription` follows it. Scope release is the mechanism here, not an omission — see `choose-static-vs-manual-subscribers-deliberately.md`, whose leak case is an instance parked on a `SingleInstance` global that never leaves scope.
|
The mirror-image anti-pattern belongs to the reviewer: flagging the `BindSubscription` here as a leaked binding because no `UnbindSubscription` follows it. Scope release is the mechanism, not an omission — see `microsoft/knowledge/events/choose-static-vs-manual-subscribers-deliberately.md`, whose leak case is an instance parked on a `SingleInstance` global that never leaves scope.
|
||||||
|
|
||||||
See sample: `expose-process-context-via-manually-bound-flag.bad.al`.
|
See sample: `expose-process-context-via-manually-bound-flag.bad.al`.
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue