Skip to content

🐛 use 32bit integers for threadid in protocol layer #761

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

Merged
merged 67 commits into from
Feb 12, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
67 commits
Select commit Hold shift + click to select a range
ecc1ca9
Fix Microsoft/vscode#37627 (#1368)
octref Nov 3, 2017
7c5778c
Version 0.7.0 of extension (#1381)
DonJayamanne Nov 9, 2017
9d1bf82
Update README.md
DonJayamanne Nov 9, 2017
ffba179
Update README.md
DonJayamanne Nov 9, 2017
905c713
sync fork with upstream
DonJayamanne Nov 10, 2017
acc2109
fix readme
DonJayamanne Nov 10, 2017
d470523
Merge branch 'master' of https://github.com/Microsoft/vscode-python
DonJayamanne Nov 16, 2017
d392e8b
merged upstream
DonJayamanne Nov 16, 2017
92f775f
Merge remote-tracking branch 'upstream/master'
DonJayamanne Nov 20, 2017
32a6e53
Merge remote-tracking branch 'upstream/master'
DonJayamanne Nov 21, 2017
4b30f2c
Merge remote-tracking branch 'upstream/master'
DonJayamanne Nov 22, 2017
e396752
Merge remote-tracking branch 'upstream/master'
DonJayamanne Nov 22, 2017
eff4792
Merge remote-tracking branch 'upstream/master'
DonJayamanne Nov 28, 2017
4553c28
Merge remote-tracking branch 'upstream/master'
DonJayamanne Nov 28, 2017
3c6520a
Merge remote-tracking branch 'upstream/master'
DonJayamanne Nov 28, 2017
966e516
Merge remote-tracking branch 'upstream/master'
DonJayamanne Nov 28, 2017
63d2d65
Merge remote-tracking branch 'upstream/master'
DonJayamanne Nov 28, 2017
f6d469e
Merge remote-tracking branch 'upstream/master'
DonJayamanne Nov 28, 2017
029e055
Merge remote-tracking branch 'upstream/master'
DonJayamanne Nov 30, 2017
e8c71c0
Merge remote-tracking branch 'upstream/master'
DonJayamanne Nov 30, 2017
51cf9d2
Merge remote-tracking branch 'upstream/master'
DonJayamanne Dec 1, 2017
7aadc43
Merge remote-tracking branch 'upstream/master'
DonJayamanne Dec 1, 2017
f0f5c59
Merge remote-tracking branch 'upstream/master'
DonJayamanne Dec 4, 2017
b2b9da9
Merge remote-tracking branch 'upstream/master'
DonJayamanne Dec 4, 2017
30a4091
Merge remote-tracking branch 'upstream/master'
DonJayamanne Dec 5, 2017
b16d2f9
Merge remote-tracking branch 'upstream/master'
DonJayamanne Dec 6, 2017
c8db345
Merge remote-tracking branch 'upstream/master'
DonJayamanne Dec 7, 2017
0df7f16
Merge remote-tracking branch 'upstream/master'
DonJayamanne Dec 8, 2017
3ccc881
Merge remote-tracking branch 'upstream/master'
DonJayamanne Dec 9, 2017
bb0709e
Merge remote-tracking branch 'upstream/master'
DonJayamanne Dec 11, 2017
2c19004
Merge remote-tracking branch 'upstream/master'
DonJayamanne Dec 11, 2017
8f224ab
Merge remote-tracking branch 'upstream/master'
DonJayamanne Dec 11, 2017
41b7080
Merge remote-tracking branch 'upstream/master'
DonJayamanne Dec 12, 2017
dab38dc
Merge remote-tracking branch 'upstream/master'
DonJayamanne Dec 12, 2017
ae22dd4
Merge remote-tracking branch 'upstream/master'
DonJayamanne Dec 12, 2017
d2340d2
Merge remote-tracking branch 'upstream/master'
DonJayamanne Dec 14, 2017
52bb7ae
Merge remote-tracking branch 'upstream/master'
DonJayamanne Dec 15, 2017
b6b2531
Merge remote-tracking branch 'upstream/master'
DonJayamanne Dec 19, 2017
8d8d2fc
Merge remote-tracking branch 'upstream/master'
DonJayamanne Dec 19, 2017
c425a55
Merge remote-tracking branch 'upstream/master'
DonJayamanne Jan 3, 2018
3963217
Merge remote-tracking branch 'upstream/master'
DonJayamanne Jan 3, 2018
a696f2a
Merge remote-tracking branch 'upstream/master'
DonJayamanne Jan 3, 2018
a31e659
Merge remote-tracking branch 'upstream/master'
DonJayamanne Jan 4, 2018
2663cd5
Merge remote-tracking branch 'upstream/master'
DonJayamanne Jan 4, 2018
7c85e0b
Merge remote-tracking branch 'upstream/master'
DonJayamanne Jan 5, 2018
beb82c2
Merge remote-tracking branch 'upstream/master'
DonJayamanne Jan 9, 2018
01e722a
Merge remote-tracking branch 'upstream/master'
DonJayamanne Jan 10, 2018
d84da8e
Merge remote-tracking branch 'upstream/master'
DonJayamanne Jan 17, 2018
78da3e1
Merge remote-tracking branch 'upstream/master'
DonJayamanne Jan 19, 2018
685b683
Merge remote-tracking branch 'upstream/master'
DonJayamanne Jan 23, 2018
43364fd
Merge remote-tracking branch 'upstream/master'
DonJayamanne Jan 23, 2018
8701636
Merge remote-tracking branch 'upstream/master'
DonJayamanne Jan 24, 2018
5c8addf
Merge remote-tracking branch 'upstream/master'
DonJayamanne Jan 25, 2018
588c2f9
Merge remote-tracking branch 'upstream/master'
DonJayamanne Jan 30, 2018
66b1382
Merge branch 'master' of https://github.com/DonJayamanne/pythonVSCode
DonJayamanne Jan 30, 2018
e6b4b48
Merge remote-tracking branch 'upstream/master'
DonJayamanne Jan 31, 2018
6aa9a2d
Merge remote-tracking branch 'upstream/master'
DonJayamanne Feb 4, 2018
57f3982
Merge remote-tracking branch 'upstream/master'
DonJayamanne Feb 6, 2018
15aab64
Merge branch 'master' of https://github.com/DonJayamanne/pythonVSCode
DonJayamanne Feb 6, 2018
0e7888f
Merge remote-tracking branch 'upstream/master'
DonJayamanne Feb 8, 2018
8bc4982
Merge remote-tracking branch 'upstream/master'
DonJayamanne Feb 9, 2018
c05d035
Merge remote-tracking branch 'upstream/master'
DonJayamanne Feb 9, 2018
08476d1
Merge branch 'master' of https://github.com/DonJayamanne/pythonVSCode
DonJayamanne Feb 9, 2018
aeade06
Merge remote-tracking branch 'upstream/master'
DonJayamanne Feb 9, 2018
6850da2
Merge branch 'master' of https://github.com/DonJayamanne/pythonVSCode
DonJayamanne Feb 9, 2018
04add73
Merge remote-tracking branch 'upstream/master'
DonJayamanne Feb 11, 2018
3c86fa9
:bug: use 32bit integers for threadid in protocol layer
DonJayamanne Feb 12, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/client/debugger/Common/Contracts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ export interface IPythonThread {
Process: IPythonProcess;
Name: string;
Id: number;
Int32Id: number;
Frames: IPythonStackFrame[];
}

Expand Down
5 changes: 3 additions & 2 deletions src/client/debugger/Common/Utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,13 +38,14 @@ export function validatePathSync(filePath: string): boolean {
return exists;
}

export function CreatePythonThread(id: number, isWorker: boolean, process: IPythonProcess, name: string = ''): IPythonThread {
export function CreatePythonThread(id: number, isWorker: boolean, process: IPythonProcess, name: string = '', int32Id: number = 0): IPythonThread {
Copy link

@MikhailArkhipov MikhailArkhipov Feb 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in HandleThreadCreate we are passing process to utils.CreatePythonThread. It might be better to make CreatePythonThread private within PythonProcess since all we really do in HandleThreadCreate is creating thread and then adding it to the process thread list. The PythonProcess could just do it internally. You probably wouldn't need _createdFirstThread either then.

Copy link

@MikhailArkhipov MikhailArkhipov Feb 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a related note, things like

        if (this.process.Threads.has(threadId)) {
            pyThread = this.process.Threads.get(threadId)!;
        }

could be encapsulated inside PythonProcess method since sequence appears to be repeated quite a bit. Moveover

        let threadId = this.stream.ReadInt64();
        if (this.stream.HasInsufficientDataForReading) {
            return;
        }
        let pyThread: IPythonThread | undefined;
        if (this.process.Threads.has(threadId)) {
            pyThread = this.process.Threads.get(threadId)!;
        }

appears to be used a lot too. Maybe make getThreadFromStream

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reluctant to make any changes, as all of the work would be obsolete with the new debugger work.

return {
IsWorkerThread: isWorker,
Process: process,
Name: name,
Id: id,
Frames: []
Frames: [],
Int32Id: int32Id
};
}

Expand Down
21 changes: 10 additions & 11 deletions src/client/debugger/Main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -145,21 +145,21 @@ export class PythonDebugger extends LoggingDebugSession {
this.stopDebugServer();
}
private onPythonThreadCreated(pyThread: IPythonThread) {
this.sendEvent(new ThreadEvent("started", pyThread.Id));
this.sendEvent(new ThreadEvent("started", pyThread.Int32Id));
}
private onStepCompleted(pyThread: IPythonThread) {
this.sendEvent(new StoppedEvent("step", pyThread.Id));
this.sendEvent(new StoppedEvent("step", pyThread.Int32Id));
}
private onPythonException(pyThread: IPythonThread, ex: IPythonException) {
this.lastException = ex;
this.sendEvent(new StoppedEvent("exception", pyThread.Id, `${ex.TypeName}, ${ex.Description}`));
this.sendEvent(new StoppedEvent("exception", pyThread.Int32Id, `${ex.TypeName}, ${ex.Description}`));
this.sendEvent(new OutputEvent(`${ex.TypeName}, ${ex.Description}\n`, "stderr"));
}
private onPythonThreadExited(pyThread: IPythonThread) {
this.sendEvent(new ThreadEvent("exited", pyThread.Id));
this.sendEvent(new ThreadEvent("exited", pyThread.Int32Id));
}
private onPythonProcessPaused(pyThread: IPythonThread) {
this.sendEvent(new StoppedEvent("user request", pyThread.Id));
this.sendEvent(new StoppedEvent("user request", pyThread.Int32Id));
}
private onPythonModuleLoaded(module: IPythonModule) {
}
Expand All @@ -181,7 +181,7 @@ export class PythonDebugger extends LoggingDebugSession {
// tslint:disable-next-line:no-non-null-assertion
const thread = pyThread!;
if (this.launchArgs && this.launchArgs.stopOnEntry === true) {
this.sendEvent(new StoppedEvent("entry", thread.Id));
this.sendEvent(new StoppedEvent("entry", thread.Int32Id));
} else if (this.launchArgs && this.launchArgs.stopOnEntry === false) {
this.configurationDone.then(() => {
this.pythonProcess!.SendResumeThread(thread.Id);
Expand Down Expand Up @@ -320,7 +320,7 @@ export class PythonDebugger extends LoggingDebugSession {
private onBreakpointHit(pyThread: IPythonThread, breakpointId: number) {
// Break only if the breakpoint exists and it is enabled
if (this.registeredBreakpoints.has(breakpointId) && this.registeredBreakpoints.get(breakpointId)!.Enabled === true) {
this.sendEvent(new StoppedEvent("breakpoint", pyThread.Id));
this.sendEvent(new StoppedEvent("breakpoint", pyThread.Int32Id));
}
else {
this.pythonProcess!.SendResumeThread(pyThread.Id);
Expand Down Expand Up @@ -429,7 +429,7 @@ export class PythonDebugger extends LoggingDebugSession {
const threads: Thread[] = [];
if (this.pythonProcess) {
this.pythonProcess!.Threads.forEach(t => {
threads.push(new Thread(t.Id, t.Name));
threads.push(new Thread(t.Int32Id, t.Name));
});
}

Expand Down Expand Up @@ -466,14 +466,13 @@ export class PythonDebugger extends LoggingDebugSession {
}
protected stackTraceRequest(response: DebugProtocol.StackTraceResponse, args: DebugProtocol.StackTraceArguments): void {
this.debuggerLoaded.then(() => {
if (this.terminateEventSent || !this.pythonProcess || !this.pythonProcess!.Threads.has(args.threadId)) {
if (this.terminateEventSent || !this.pythonProcess || Array.from(this.pythonProcess!.Threads.values()).findIndex(t => t.Int32Id === args.threadId) === -1) {
response.body = {
stackFrames: []
};
return this.sendResponse(response);
}

const pyThread = this.pythonProcess!.Threads.get(args.threadId)!;
const pyThread = Array.from(this.pythonProcess!.Threads.values()).find(t => t.Int32Id === args.threadId)!;
Copy link

@MikhailArkhipov MikhailArkhipov Feb 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth encapsulating search into IPythonProcess.threadFromIn32Id()

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do

let maxFrames = typeof args.levels === "number" && args.levels > 0 ? args.levels : pyThread.Frames.length - 1;
maxFrames = maxFrames < pyThread.Frames.length ? maxFrames : pyThread.Frames.length;

Expand Down
5 changes: 3 additions & 2 deletions src/client/debugger/PythonProcessCallbackHandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { SocketStream } from "../common/net/socket/SocketStream";
import { ExtractTryStatements } from "./Common/TryParser";
import * as path from "path";
import { IdDispenser } from '../common/idDispenser';
import '../../client/common/extensions';

export class PythonProcessCallbackHandler extends EventEmitter {
private process: IPythonProcess;
Expand Down Expand Up @@ -113,7 +114,7 @@ export class PythonProcessCallbackHandler extends EventEmitter {
return;
}

let pyThread = utils.CreatePythonThread(threadId, this._createdFirstThread, this.process);
let pyThread = utils.CreatePythonThread(threadId, this._createdFirstThread, this.process, '', this.process.Threads.size + 1);
this._createdFirstThread = true;
this.process.Threads.set(threadId, pyThread);
this.emit("threadCreated", pyThread);
Expand Down Expand Up @@ -220,7 +221,7 @@ export class PythonProcessCallbackHandler extends EventEmitter {
});
this.stream.WriteString("-");
});
});
}).ignoreErrors();
}
private GetHandledExceptionRanges(fileName: string): Promise<{ startLine: number, endLine: number, expressions: string[] }[]> {
return ExtractTryStatements(fileName).then(statements => {
Expand Down
21 changes: 21 additions & 0 deletions src/test/debugger/misc.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ use(chaiAsPromised);
const debugFilesPath = path.join(__dirname, '..', '..', '..', 'src', 'test', 'pythonFiles', 'debugging');

const DEBUG_ADAPTER = path.join(__dirname, '..', '..', 'client', 'debugger', 'Main.js');
const MAX_SIGNED_INT32 = Math.pow(2, 31) - 1;

// tslint:disable-next-line:max-func-body-length
suite('Standard Debugging - Misc tests', () => {
Expand Down Expand Up @@ -103,6 +104,23 @@ suite('Standard Debugging - Misc tests', () => {
debugClient.waitForEvent('terminated')
]);
});
test('Ensure threadid is int32', async () => {
const threadIdPromise = debugClient.waitForEvent('thread');

await Promise.all([
debugClient.configurationSequence(),
debugClient.launch(buildLauncArgs('simplePrint.py', true)),
debugClient.waitForEvent('initialized'),
debugClient.waitForEvent('stopped')
]);

const threadId = ((await threadIdPromise) as ThreadEvent).body.threadId;
expect(threadId).to.be.lessThan(MAX_SIGNED_INT32 + 1, 'ThreadId is not an integer');
await Promise.all([
debugClient.continueRequest({ threadId }),
debugClient.waitForEvent('terminated')
]);
});
test('Should break at print statement (line 3)', async () => {
const launchArgs = buildLauncArgs('sample2.py', false);
const breakpointLocation = { path: path.join(debugFilesPath, 'sample2.py'), column: 0, line: 3 };
Expand Down Expand Up @@ -372,6 +390,9 @@ suite('Standard Debugging - Misc tests', () => {

const threads = await debugClient.threadsRequest();
expect(threads.body.threads).of.lengthOf(2, 'incorrect number of threads');
for (const thread of threads.body.threads) {
expect(thread.id).to.be.lessThan(MAX_SIGNED_INT32 + 1, 'ThreadId is not an integer');
}
});
test('Test stack frames', async () => {
await Promise.all([
Expand Down