#server crash when too much request on one controller

18 messages · Page 1 of 1 (latest)

wary forge
#

Hello! On my current project, I'm working on a password-less connection; I have a usecase class to handle connection code creation. I have a method that creates code, and it checks if there's already an existing code, if so, it replaces it with a new one. The problem is that when I send too many requests in a short time, it crashes on this method. It returns me an error: The key already exists (Because there is a constraint on my SQL table, and there can be only one code per email). And I don't want my server to crash just for that. I don't know if that's an async error. I guess I can add a middleware to limit requests but I don't know if there's a better way to fix that...

Here's my current methods, if someone think of a way to improve it, I'd like it!

async CreateCode(email: string, userId: string): Promise<Code | Error> {
    const code = Math.floor((Math.random() * 899999) + 100000);
    const newCode: Code = {
      code: code,
      email: email,
      expiresAt: Date.now() + 15 * 60 * 1000,
      userId: userId
    }
    try {
      const existingCode = await this.GetCodeByEmail(email);

      if (!(existingCode instanceof Error)) {
          const deletionError = await this.DeleteCode(existingCode.code);
          if (deletionError) {
              return new ErrorWithCode(deletionError.message, 500);
          }
      }

      await this.codeRepository.InsertOne(newCode);

      return newCode;
    } catch (error) {
        if (error instanceof Error) {
            return new ErrorWithCode(error.message, 500);
        }
        return Promise.resolve(null as unknown as Code);
      }
  }
lost condor
#

hard to know what's wrong without any actual error message, or knowing what lib you are using for the db connection

#

also that code be improved quite a bit 👀

wary forge
lost condor
#

that's strictly a db error

#

you are using the email as primary key, which won't work if the same user can recieve multiple codes over time

#

also, if the server crashes, it means there is an issue elsewhere in your handler
the part of the code you showed seems fine at handling errors

wary forge
wary forge
lost condor
#

a few things to say about your code tho

  • JS/TS uses camelCase as naming convention for the function methods
  • don't handle the errors in that "utility" method,handle them at a higher level
  • don't return error, use the buitin throw system, and use a catch block where appropriate; don't go against the language's paradigm
  • don't mix return type, that method was returning Code | null | ErrorWithCode, that's too many types; either it returns a Code or it doesn't and throws
reef sailBOT
#
ascor8522#0
Playground Link

Preview:```ts
import {randomInt} from "node:crypto"

declare interface Code {
code: number
email: string
expiresAt: number
userId: string
}

declare interface CodeRepository {
insertOne(code: Code): Promise<void>
}

class CodeService {
private static readonly CODE_DURATION_MS =
15 *
...```

You can choose specific lines to embed by selecting them before copying the link.
wary forge
#

Also, I've found a way to fix my error. Just update my sql query to handle conflict, instead of

      this.database.query(`insert into codes(code, email, expires_at, user_id) values (${code.code}, '${code.email}', ${code.expiresAt}, '${code.userId}')`)

I wrote that:

      this.database.query(`insert into codes(code, email, expires_at, user_id) values (${code.code}, '${code.email}', ${code.expiresAt}, '${code.userId}') on conflict (email) do update SET code = ${code.code}, expires_at = ${code.expiresAt}`)
#

And that's it; I'm not used to not using ORM, so yeah. I feel dumb, but that's ok

#

!resolved

hearty ruin
#

@wary forge you may want to read up on transactions as well if you encounter this kind of problem in the future (though for this specific situation i think your solution makes sense—assuming last-update-wins is the behavior you want)

#

this is tangential, but you should also be using parameterized queries rather than string interpolation to avoid the possibility of SQL injection attacks