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

User id module: adds a catch to greedy promise #10983

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mmoschovas
Copy link
Contributor

Type of change

  • Bugfix

  • Feature

  • New bidder adapter

  • Code style update (formatting, local variables)

  • Refactoring (no functional changes, no api changes)

  • Build related changes

  • CI related changes

  • Does this change affect user-facing APIs or examples documented on http://prebid.org?

  • Other

Description of change

Errors that occur currently throw an Uncaught promise error on page. This adds a catch to avoid the error and logs as well for debugging

Other information

…urrently throw an Uncaught promise error on page
@patmmccann patmmccann requested a review from dgirardi January 24, 2024 19:56
@patmmccann patmmccann self-requested a review January 24, 2024 19:57
@patmmccann patmmccann self-assigned this Jan 24, 2024
@patmmccann patmmccann changed the title adds a catch to greedy promise in user id module User id module: adds a catch to greedy promise Jan 24, 2024
@@ -488,6 +488,7 @@ function idSystemInitializer({delay = GreedyPromise.timeout} = {}) {
}
cancel = defer();
return GreedyPromise.race([promise, cancel.promise])
.catch(() => logWarn(`${MODULE_NAME} - cancelled promise`))
Copy link
Collaborator

Choose a reason for hiding this comment

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

as discussed offline, I am not able to repro the issue so I am grasping at straws here - instead of this, could you try replacing

    if (cancel != null) {
      cancel.reject(INIT_CANCELED);
    }

with

    ((c) => {
      setTimeout(() => {
        if (c != null) {
          c.reject(INIT_CANCELED);
        }
      })
    })(cancel);

and see if it makes any difference? (this change also fails tests but I think that's a problem with the tests so if it works I can go ahead and look at them as well).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesnt appear that worked, if we change from logWarn to logError is that more palatable?

Copy link
Collaborator

Choose a reason for hiding this comment

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

it seems

function refreshUserIds({submoduleNames} = {}, callback) {
may trigger the cancel

@coreyb-cbs
Copy link

coreyb-cbs commented Jun 5, 2024

@dgirardi @patmmccann Not sure if it's helpful, but I experienced this uncaught promise error when calling refreshUserIds twice in quick succession. I was able to eliminate the error by removing one of those calls (it wasn't necessary anymore). It was on my to-do list to raise this up in a Git issue to have the promise fail more gracefully or provide some sort of error context. I was able to reproduce consistently in Chrome. I can probably dig up an old bundle if that's helpful, let me know.

I am able to reproduce on every page load:

  1. set up a network request redirect from https://at.cbsi.com/lib/dist/prod/bidbarrel-maxpreps.min.js to https://at.cbsi.com/lib/dist/v3.19.0/all/bidbarrel-maxpreps.js
  2. visit https://www.maxpreps.com
  3. check local storage fmscw_resp for value.fms_params.fms_UID2
  4. if it doesn't exist then you need to log in with an account
  5. once that fms_UID2 object exists, check the console error logs for Uncaught (in promise) {}

@dgirardi
Copy link
Collaborator

dgirardi commented Oct 7, 2024

@coreyb-cbs could you verify if the issue still happens after #12246?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants