Skip to content

Removal of sync functions #32

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

Closed
markdirish opened this issue Jan 30, 2019 · 5 comments
Closed

Removal of sync functions #32

markdirish opened this issue Jan 30, 2019 · 5 comments
Assignees
Milestone

Comments

@markdirish
Copy link
Contributor

In the currently published package, there is the option to do synchronous calls to the various functions in the iDataQueue, iNetwork, iObj, iProd, iUserSpace, and iWork.

As currently implemented, calling synchronously is a mess. It looks like the code creates a timeout to check if the result has been returned. If not, it creates ANOTHER timeout to check if the result is returned, spinning more and more timeouts until it either returns or timesout. Just doing static analysis of the code, I'm not even convinced it returns correctly, and I believe there has been customer confusion in the past when trying to use this package synchronously.

Because no one should be using this package synchronously anyway, @abmusse and I believe that we should remove all that logic and, for now, assume that the user is passing a callback function. Once we get that baked in, we can look at using more modern asynchronous methods like Promises.

@markdirish markdirish self-assigned this Jan 30, 2019
@markdirish markdirish added this to the Version 1.0 milestone Jan 30, 2019
@abmusse
Copy link
Member

abmusse commented Jan 30, 2019

I see the process has already started with 7fdf0fd on v1.0-dev branch.

Thanks @markdirish !

@kadler
Copy link
Member

kadler commented Jan 30, 2019

FYI, @abmusse and I ran through the code sequence and since calling the transport with the sync option set to true causes it to be synchronous, it will not return until it has a value and called the callback, which means that the whole timeout and retry logic is pointless. Indeed, if it was not the case that stop was set to 1 before waitForTimeout was run, there would be no way to retrieve the result (as waitForResult would return the result to setTimeout instead of the actual caller.

@markdirish
Copy link
Contributor Author

Yeah, it was a mess. Even if we decide to keep sync methods, the way it was written before needed to be excised either way. Good info.

@kadler
Copy link
Member

kadler commented Jan 30, 2019

I doubt anyone cares about sync, and if they do they could wrap it in a promise or something.

abmusse added a commit that referenced this issue Mar 11, 2019
- We no longer support sync mode which had an option to set timeout
@abmusse
Copy link
Member

abmusse commented Jun 13, 2019

Resolved by 7fdf0fd

@abmusse abmusse closed this as completed Jun 13, 2019
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

No branches or pull requests

3 participants