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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use native promises #1171

Merged
merged 15 commits into from
Jul 7, 2022
Merged

Use native promises #1171

merged 15 commits into from
Jul 7, 2022

Conversation

carloslbello
Copy link
Collaborator

Refactored the code to use native promises and remove a few dependencies.

@idinium96
Copy link
Member

Thank you. I will have a look on this and solve the conflict once I'm back home.

@idinium96 idinium96 changed the base branch from development to v5-dev June 17, 2022 12:38
@idinium96
Copy link
Member

Works now.
Previously it just never initializes bptf-listings.

Copy link
Member

@idinium96 idinium96 left a comment

Choose a reason for hiding this comment

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

Looks like the process is not terminated when there's an error on start, and it will just be stuck there indefinitely.
Please check for missing catch or throw. Should crash/terminate the process if there's an error while starting.
image

Copy link
Member

@idinium96 idinium96 left a comment

Choose a reason for hiding this comment

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

Stopping the bot also does not immediately terminate the process while it's starting, like how it used to be.
image

src/classes/Bot.ts Outdated Show resolved Hide resolved
@carloslbello
Copy link
Collaborator Author

I think returning the promise from the start function should fix the error handling issue. Just added a commit which does that.

@idinium96
Copy link
Member

idinium96 commented Jun 17, 2022

nah.
image

I think should have catch here.

tf2autobot/src/app.ts

Lines 144 to 152 in aaf13c5

void (async () => {
await botManager.start(options);
if (options.enableHttpApi) {
const { default: HttpManager } = await import('./classes/HttpManager');
const httpManager = new HttpManager(options);
await httpManager.start();
}
})();

.catch(err => throw err));

Or do try-catch method

@idinium96 idinium96 mentioned this pull request Jun 17, 2022
11 tasks
@idinium96
Copy link
Member

The process is still not terminated/crashed when there's an error on start.
image

@carloslbello
Copy link
Collaborator Author

Can you place a log statement before line 734 in src/classes/Bot.ts to see if the checkIfStopping function is being run after each promise in the chain?

@idinium96
Copy link
Member

Sure thing. Let me try.
Also, if you want to use my extra account reserved for testing, just let me know.

@idinium96
Copy link
Member

image

The old one should be here that will reject the start

tf2autobot/src/classes/Bot.ts

Lines 1028 to 1031 in f8f7c9a

err => {
if (err) {
return reject(err);
}

I am also not sure if using let promise = Promise.resolve(); is the right way to do or not.

@idinium96
Copy link
Member

Maybe bring back

return new Promise((resolve, reject) => {

return new Promise((resolve, reject) => {

and reject on error, so that it will catch the error in app.js

@carloslbello
Copy link
Collaborator Author

I am unable to reproduce these issues on my machine at the current commit. I tried with node v16.15.1 and node v18.4.0.

@idinium96
Copy link
Member

idinium96 commented Jun 18, 2022

If you're unable to reproduce the error, that means Steam is back alive.
Try adding a line that will throw an error anywhere in the promiseChains, like the one I previously did (the "nah." one)

@carloslbello
Copy link
Collaborator Author

I can't reproduce even when manually introducing an error - the code correctly shuts down upon erroring on my machine. See the attached mov file.

Screen.Recording.2022-06-19.at.7.47.42.PM.mov

@idinium96
Copy link
Member

hhmmmm weird.

@idinium96 idinium96 changed the base branch from v5-dev to development July 7, 2022 00:56
@idinium96
Copy link
Member

Merging this into development.
Thank you very much!

@idinium96 idinium96 merged commit a5f5eea into development Jul 7, 2022
@idinium96 idinium96 mentioned this pull request Jul 8, 2022
@idinium96 idinium96 deleted the native-promises branch July 8, 2022 13:35
@idinium96 idinium96 mentioned this pull request Jul 20, 2022
idinium96 added a commit that referenced this pull request Nov 25, 2022
idinium96 added a commit that referenced this pull request Jan 2, 2023
↩️ Partially revert changes made in #1171
@idinium96 idinium96 mentioned this pull request Jan 3, 2023
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.

2 participants