Skip to content
This repository was archived by the owner on Apr 14, 2024. It is now read-only.

Conversation

@chetanyakan
Copy link
Contributor

@chetanyakan chetanyakan commented Oct 8, 2019

Summary
Prevent creation of a corrupted file if the file conversion fails.

Explain the motivation for making this change. What existing problem does the pull request solve?

I tried converting a ppt file to pdf. Due to some memory or time issue (I wasn't able to figure out the cause), the file failed to convert and a corrupted file of ~57B was written instead, which could not be opened in a PDF viewer.

For reference, the following error was in the Gotenberg logs:

code=500, message=unoconv: non-zero exit code: exit status 6

I expected the client.Store() method to return an error instead of writing a corrupted file.

Test plan (required)

Demonstrate the code is solid. Example: The exact commands you ran and their output.

Checklist

  • Have you followed the guidelines in our CONTRIBUTING guide?
  • Have you lint your code locally prior to submission (make lint)?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally (make tests)?
  • I have squashed any insignificant commits
  • This change has comments for package types, values, functions, and non-obvious lines of code

@gulien
Copy link
Contributor

gulien commented Oct 8, 2019

Hello @chetanyakan

Thank you for this PR.

I expected the client.Store() method to return an error instead of writing a corrupted file.

Indeed, I was expected the line resp, err := c.Post(req) to return an error if status != 200 (I'm not a familiar with Golang HTTP client..).

Concerning your conversion issue, did you use the latest version of Gotenberg?

client.go Outdated
defer resp.Body.Close()

// Check for 2XX Status Codes
if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not simply check res.StatutsCode != http.StatusOK?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed I was not aware whether gotenberg returns any non-200 (2XX) status codes upon successful conversion to pdf.
If the status code is expected to be 200 always, you can let me know and I can make the required change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes the expected status code is always excepted to be 200 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@chetanyakan
Copy link
Contributor Author

Regarding my issue, I was using gotenberg v4.
It was likely some memory-related issue as although I was able to repetedly see the issue before, I was able to successfully convert the same file afterward with same version of gotenberg.

@chetanyakan chetanyakan requested a review from gulien October 8, 2019 19:25
@gulien
Copy link
Contributor

gulien commented Oct 9, 2019

It was likely some memory-related

Version 6 should behave way better on that issue 👍

@gulien
Copy link
Contributor

gulien commented Oct 9, 2019

Thank you 😄 👍

@gulien gulien merged commit 921a34c into thecodingmachine:master Oct 9, 2019
@chetanyakan chetanyakan deleted the MI-380 branch October 1, 2020 07:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants