Fixed race condition on first start-up

[ksosa523]

I noticed I'm getting an error of "Connection is closed" on first starting up the express Node app.

The store functions properly and all works well, but it is an annoying error that always shows up on app-start up. After testing it out, it seems like it's a race condition, and adding a small delay of 0.05 seconds fixes this issue. That is what this PR is for.

[JLuboff]

Can you provide a way to replicate this issue? I haven't seen this issue be brought up recently so want to be able to replicate before reviewing/merging. Thanks!

[ksosa523]

Can you provide a way to replicate this issue? I haven't seen this issue be brought up recently so want to be able to replicate before reviewing/merging. Thanks!

Unfortunately, I'm not sure how I can provide a way to replicate this without exposing sensitive information since I work for the county's school district. Totally understandable if you don't feel comfortable merging this PR. I can continue to just include that line in our internal apps, by forking your package and adding it into our personal NPM repository. Thank you for the response!

[bradtaniguchi]

I'm against knowingly slowing down code performance. I've seen race condition related startup issues when it comes due to multiple requests being made at the same point in time, while waiting for a shared "resolution". So in this case, its possible the dbReadyCheck actually doesn't handle multiple simultaneous requests, but this is just an assumption.

My assumption is something similar is happening here, but its hard to tell without any reproduction code.

@ksosa523 you can help confirm this behavior by just confirming if your code makes multiple SQL calls initially? Something like two gets being called back-to-back or in aPromise.all or at roughly the same time would help point us in the right direction.

[ksosa523]

@bradtaniguchi Sure, this is really all my code does in terms of initializing the store with a session. I'm simply just instantiating it, and then starting up the express with app.use and storing the store into the session:

const store = new MSSQLStore({
  user: "<SQL_USER_ACC>",
  password: "<SQL_PASSWORD>",
  server: "<SERVER_INFO_HERE>",
  database: "NodeState",
  options: {
    autoRemove: true,
    autoRemoveInterval: 28800000,
    trustServerCertificate: true,
  },
});

app.use(
  session({
    store: store as Store,
    secret: "secret",
    resave: true,
    saveUninitialized: false,
    // expires in 8 hours
    cookie: {
      maxAge: 28800000,
    },
  }),
);

[bradtaniguchi]

Thanks for providing those pieces of code!

I have a few more questions.

Does your server start initially on a request, or does it "sit waiting" 24/7 and the code never has to restart.

I ask primarily to cover the scenario where the server is starting up and immediately gets hit by a session check for a request, at which point the connection to MYSQL might not be setup yet. This sort of setup is more common in situations where the code is running on infrastructure that scales up and down often.

You originally mentioned that this error appears on startup, but I'd like to understand what circumstances are included in that "startup".

Thanks!

[ksosa523]

Thank you! That helped fix my problem - it turns out I was calling to save the session twice without awaiting the other one, and that caused this issue. I apologize for opening the PR and having to use your guys' time, but thank you so much for your tips. Works great now!

[bradtaniguchi]

Glad to hear you figured it out 👍🏼 and also thanks for reaching out to contribute to the library regardless of the outcome 😄

[JLuboff]

Thank you both for your diligence on this!