Pnpjs: Underlying SPFx token provider occasionally hangs without resolving or rejecting

[TazzyMan]

Category

• [ ] Enhancement
• [x] Bug
• [ ] Question
• [ ] Documentation gap/issue

Version

@pnp/pnpjs ~1.3.3
SharePoint Online - SPFx

I didn't update yet, but based on the source code I can see the issue isn't resolved yet

Expected / Desired Behavior / Question

Get token from SharePoint and either fail or succeed

Observed Behavior

Token is requested and promise never returns, killing the promise chain alltogether

Steps to Reproduce

When entering the page from a link in Modern sharepoint (not a full page refresh) or when all cookies etc are cleared. Probably a race condition in the Modern SharePoint page, but I would at least expect an error in that case. The source code indicates it doesn't handle errors well.
The specific code is below:

    /**
     * Gets an AAD token for the provided resource using the SPFx AADTokenProvider
     * 
     * @param resource Resource for which a token is to be requested (ex: https://graph.microsoft.com)
     */
    public getToken(resource: string): Promise<string> {

        return this.context.aadTokenProviderFactory.getTokenProvider().then(provider => {

            return provider.getToken(resource);
        });
    }

[patrick-rodgers]

Hi @TazzyMan - if I am understanding you correctly the issue is contained within the SPFx aadTokenProviderFactory? If so there isn't much we can do to fix that, but you could report that issue to the sp-dev-docs issues list.

That being said, we could add a timeout here to produce an error if we don't get back a token within some amount of time - would that help?

[TazzyMan]

Hello Patrick,

As a matter of fact I already resolve the issue with a timeout, but there are cases where it takes up to 70 retries to get that token, which each a 1000ms delay, so I'd rather have the exception.
When I look at the code I miss a .catch() in both the call to getTokenProvider() and getToken() and there are no means of rejecting the promise. I didn't have time to test it yet, so that I could do a pull request but I would expect the code to be like below:

    public getToken(resource: string): Promise<string> {
        return new Promise<string>((resolve, reject) => {
            this.context.aadTokenProviderFactory.getTokenProvider()
             .then((provider:any) => {
                provider.getToken(resource)
                .then((token:string) => {
                    resolve(token);
                })
                .catch((error:string) => {
                   reject(error);
                });
            ).catch((error:string) => {
               reject(error);
            });
        });
    }

Off course, when either getTokenProvider() or getToken() also makes the promise die, we still have the same issue, but then at least the promise is handled correctly. In that case you could still add some timeout mechanism with retries to help other users.

[patrick-rodgers]

Not sure I follow that, if there is an error you have the ability to handle and catch it yourself. In the current method we return to you the promise chain. You can then use .catch or try..catch with await to get any errors. I thought the issue was that nothing was returned because the method didn't produce and error OR a result. Adding extra .catch calls into the the chain wouldn't fix that issue. What I mean:

return this.context.aadTokenProviderFactory.getTokenProvider().then(provider => {
    return provider.getToken(resource);
});

That entire statement returns a promise instance. ANY error in that chain SHOULD produce a catchable error - no need to inject catches into the chain.

With that understanding, is there an issue here for us to address? Would us including a timeout into the method that would throw an error out of that method if there is no result help?

[TazzyMan]

Hello Patrick,

When in this case the getTokenProvider() fails, this isn't communicated in the promise. The promise isn't rejected and therefore the promise dies because of that (no resolve or reject). When it succeeds getToken() is returned, which by itself is a promise, so that one could communicate a failure when needed.
The code above was an example of gracefully handling all the promises, but at least a failure for getTokenProvider() should be handled.
Alternatively you could let typescript handle it by using the async / await pattern.

The behavior I observed was a the promise to SPFxAdalClient.getToken() was never resolved in those cases. Because it didn't return I also didn't know if the Microsoft part getToken() or even getTokenProvider() killed the promise. That's why I looked up the code on github and noticed there was no way for that method to return a failure. Later I learned getToken() was also a promise.

[KEMiCZA]

@TazzyMan @patrick-rodgers I created a PR for this. Can you review if this would solve the problem. I did not have time to test the code, but I assume this is what TazzyMan was trying to describe.

EDIT: as Patrick and Andrew mentioned the async and native promise code should have the same result. Ignore my PR, was a bit confused.

[TazzyMan]

@KEMiCZA As far as I can see this would fix the problem. It's true that async / await does exactly the same as .then() and .catch(), but it does so automatically. Even when you don't explicity catch the error. The real problem is when getTokenProvider() would fail. There is no .catch() and reject() in the original code. Because async/await is nowhere in this class I would, in this case, use .then() and .catch(0 instead, as the generated class is a little harder to debug in javasript with async/await.

At this moment I cannot tell which promise fails, because packages are minified, and that would be fixed by this change. I will soon run some test with a overriden SPFxAdalClient and see if that helps. I will let you know

I'm new to PR's in github. It was closed. Does that mean your change is rejected or accepted?

[KEMiCZA]

I'm new to PR's in github. It was closed. Does that mean your change is rejected or accepted?

@TazzyMan I closed it because the code would output the same

[TazzyMan]

@KEMiCZA

@TazzyMan I closed it because the code would output the same
But AFAIK that is not the case. When getTokenProvider() fails the promise will die in the old code. The new code resolves that because async/await always rejects the promise when an exception occurs.
A different approach would be to add a .catch() to the getTokenProvider() and make the promise capable of resolving and rejecting the promise (see my example). In that case the output would be the same.

[patrick-rodgers]

I am not sure how to be clearer about this but that entire function outputs a promise, which is the combined chain of those two calls. Then ".then" construct chains all that together - any error at any point in the chain would produce a catchable error. Meaning if there is an error and you have a ".catch" or are using try/catch and await pattern you will catch it in your code. Rewriting the method to include extra .catch statements or using async await does nothing to change this behavior.

IF the issue you are trying to report is that one of the SPFx token provider methods never resolves or rejects that is a different issue. Which is why I suggested that _if that is the underlying issue_ it should be reported to the sp-dev-docs issues list and we could look to add a timeout that would force an error if the existing chain doesn't return in some amount of time.

Please answer on the second point, is this the issue you are trying to report?

[TazzyMan]

Hello Patrick,

At the moment I'm unable to reproduce the error on my machine, but our customers are able to do it somehow. Anyway then maybe I understand promises incorrectly or they have changed since the last time I've worked intensely with promises in Typescript (now I mostly use await).
AFAIK a promise dies when it fails and doesn't call reject(). On the other hand, when reject() is called, but the error isn't caught, the same thing happens.
So in this case it seems getToken() will return a promise, but ONLY if getTokenProvider() succeeds.

I first need to reproduce the problem, so that I can try the different approaches, and see if there's a difference. I will let you know.

[TheIronCheek]

For what it's worth, I'm having a similar issue with AadTokenProvider and getToken(). getToken() stalls out intermittently and when it does so, no exception is thrown. Neither my .then() nor my .catch() execute.

[patrick-rodgers]

@TheIronCheek - so you are confirming the behavior I was trying to document. As mentioned previously this should be reported in the sp-dev-docs issues list. We could add a timeout so an error is generated within our library for these cases, but we can't fix the underlying cause.

[TheIronCheek]

I mean, a timeout is something...

It'd be nice to know WHY it's timing out, though. Especially since it's so flaky. It works sometimes... but sometimes not. I don't want to hijack @TazzyMan's thread but for me, it seems related to inactivity. If I attempt to access the site after a period of inactivity, getToken() fails. If I refresh the page a few times, it'll start working again.

[patrick-rodgers]

Sure - but that code lives within SharePoint Framework, not this library so we can not change it's behavior or analyze it. The only option available to this library is a timeout based error to catch when it hangs. If the external method returns nothing we have no information to go on. I will again encourage you to report this if you haven't to the sp-dev-docs issues list so the right folks get visibility into the issue.

[TheIronCheek]

I gotcha. I misunderstood what you were saying. Thanks for your help. I reported the issue with sp-dev-docs.

[TazzyMan]

Hello @TheIronCheek and @patrick-rodgers ,

Since TheIronCreek can reproduce the error without using pnpjs and, upto now, I wasn't able to, we can safely assume aadTokenProvider causes the promise to die. I think we can close this issue then.
Thanks for all the help

[patrick-rodgers]

This is now tracked in sp-dev-docs