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

XHRUpload stuck at 100% despite 2xx response code. #5131

Closed
2 tasks done
danielclough opened this issue Apr 30, 2024 · 12 comments · Fixed by #5132
Closed
2 tasks done

XHRUpload stuck at 100% despite 2xx response code. #5131

danielclough opened this issue Apr 30, 2024 · 12 comments · Fixed by #5132
Labels

Comments

@danielclough
Copy link

Initial checklist

  • I understand this is a bug report and questions should be posted in the Community Forum
  • I searched issues and couldn’t find anything (or linked relevant results below)

Link to runnable example

No response

Steps to reproduce

I minimal reproduction can be found here:

https://github.com/danielclough/Uppy-Axum-Min-Repro---Stuck-at-100-percent

I have tried to make the README informative enough that you may not need to actually run the code to understand the problem.

Expected behavior

When the server returns a Status Code 200 the modal should close with a success message.

Actual behavior

The uploading counter is stuck at 100%

@Murderlon
Copy link
Member

Hi, I'm fairly certain the problem is in Rust, not Uppy, as I can't reproduce this with other servers and I've seen someone else having a similar problem too with Rust: #4253

@Murderlon
Copy link
Member

JS server code which works
const fs = require('fs')
const path = require('path')
const express = require('express')
const multer = require('multer')
const argv = require('minimist')(process.argv.slice(2))
const findRemoveSync = require('find-remove')

const port = Number(argv.port || process.env.PORT || 9999)
const fileFolderName = argv.folder || 'files'
const absolutePathToFileFolder = process.cwd() + '/' + fileFolderName

function cleanup() {
  console.log('Cleaning up files older that 24 hours in:', fileFolderName)
  const removedFiles = findRemoveSync(absolutePathToFileFolder, {
    files: '*.*',
    age: { seconds: 3600 * 24 },
    limit: 100,
  })
  console.log(removedFiles)
}

const app = express()

// Middleware - deal with CORS
app.use((request, response, next) => {
  response.header('Access-Control-Allow-Origin', '*')
  response.header('Access-Control-Allow-Headers', 'Origin, X-Requested-With, Content-Type, Accept')
  next()
})

// GET /files - serve uploaded files
app.use('/' + fileFolderName, express.static(absolutePathToFileFolder))

// GET / - describe what eache endpoint does
app.get('/', (request, response) => {
  response.send(`
    <html>
      <body>
        <b>POST /upload</b> - XHR upload, that will return { url } of an uploaded file<br/>
        <b>GET /files/fileName.jpg</b> - will return a file by its name from /files<br/>
        <b>POST /error</b> - endpoint that will raise an error<br/>
        <b>POST /timeout</b> - endpoint that will take 100 seconds to respond<br/><br/>
        Files are kept in: ${absolutePathToFileFolder}<br/>
      </body>
    </html>
  `)
})

// POST /upload - uploads a file to the /files directory
const mutlerInstance = multer({
  storage: multer.diskStorage({
    destination: (request, file, cb) => {
      cb(null, fileFolderName)
    },
    filename: (request, file, cb) => {
      const date = String(Date.now())
      const randomNumbers = parseInt(Math.random() * 1000000000000)
      // .extension for better browser previews
      const extension = path.extname(file.originalname)

      const filename = date + randomNumbers + extension

      cb(null, filename)
    },
  }),
})
app.post('/upload', mutlerInstance.any(), (request, response) => {
  // request.files can be undefined sometimes acc to logs.
  const file = request.files && request.files[0]

  if (!file) {
    response.json({ url: '' })
    return
  }

  // => 'http://localhost:9999'
  const currentUrl = request.protocol + '://' + request.get('host')
  // => 'files/1565118702577'
  const filePath = file.path

  // => 'http://localhost:9999/files/1565118702577'
  const url = currentUrl + '/' + filePath

  response.json({ url })
})

// POST /error - raises an error
app.post('/error', (request, response) => {
  response.status(500).json({ message: 'You completely broke the entire site!' })
})

// POST /timeout - takes 100 seconds to respond
app.post('/timeout', (request, response) => {
  setTimeout(() => {
    response.end()
  }, 100 * 1000)
})

app.listen(port, (error) => {
  if (error) {
    console.log(`Server start error: ${error}`)
  } else {
    console.log(`Server is listening on port: ${port}`)

    // Run cleanup() now and every 10 minutes
    cleanup()
    const cleanupTimer = setInterval(cleanup, 60000 * 10)

    // Create the /files folder if it doesn't already exist
    if (!fs.existsSync(fileFolderName)) {
      fs.mkdirSync(fileFolderName)
    }
    console.log('Serving files from folder:', absolutePathToFileFolder)
  }
})

Might have to do that you don't return a url property:

    Ok("Upload Success".to_string())

@Murderlon
Copy link
Member

I recently changed the code #5074 which should now throw a hard error when it can't find the url

@kevincrossgrove
Copy link

Having this same issue in a React project

@danielclough
Copy link
Author

XHRUpload docs say that the default responseType is text, which is why I returned a string.

Looking at the JS code you presented I have understood that it expects JSON of the form:

{
  "url": "https://domain.tld"
}

Fantastic project btw!

@danielclough
Copy link
Author

I was distracted by seeing @kevincrossgrove message pop up and clicked Comment instead of Close with comment.

🤦

@kevincrossgrove
Copy link

kevincrossgrove commented Apr 30, 2024

I recently changed the code #5074 which should now throw a hard error when it can't find the url

Before, The upload would finish and I could handle the response in uppy.on("complete", ...), regardless of the shape of the response from the API.

Adding a URL property to the top level of our API response is a quick fix to allow that event handler to work properly again with the original response shape (nested in another property)

@grantaveryatgfs
Copy link

grantaveryatgfs commented Apr 30, 2024

I'm running into this same issue, for my project it occurs on "@uppy/xhr-upload": "3.6.5", but not "@uppy/xhr-upload": "3.6.4".

This isn't a new project, and it was working previously to this version bump. I've isolated this issue out from the other Uppy packages in use, but here they are for reference:

    "@uppy/angular": "0.6.1",
    "@uppy/compressor": "1.1.3",
    "@uppy/core": "3.11.0",
    "@uppy/dashboard": "3.8.2",
    "@uppy/locales": "3.5.2",
    "@uppy/webcam": "3.4.1",
    "@uppy/xhr-upload": "3.6.5",

Since @danielclough's issue is apparently caused by something different (#5131 (comment)), should I make a new ticket or can this one be re-opened?

@Murderlon
Copy link
Member

@kevincrossgrove @grantaveryatgfs and returning a url is not sufficient to solve your issues?

@Murderlon
Copy link
Member

Maybe it's best if I remove the throw the url is missing, it should be there, but I didn't mean to introduce a breaking change.

@grantaveryatgfs
Copy link

@kevincrossgrove @grantaveryatgfs and returning a url is not sufficient to solve your issues?

@Murderlon actually now that I look at it more deeply, that is my issue as well.

My server in this case is a Google Cloud Platform Storage API, which has a strictly defined response object format that doesn't include a url field at the top-level: https://cloud.google.com/storage/docs/json_api/v1/objects#resource-representations . Because of that, I don't really have a way to modify the response object that Uppy receives.

If you're able to revert your change that would be great, thanks!

@Murderlon
Copy link
Member

I opened a PR, I'll release it this week.

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

Successfully merging a pull request may close this issue.

4 participants