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

Fix/wrapped reads #681

Merged
merged 2 commits into from
Aug 14, 2024

Conversation

adam-vessey
Copy link
Contributor

#659 with what seems to be the expected exception wrapping in place?

@adam-vessey adam-vessey marked this pull request as ready for review August 1, 2024 18:34
@DiegoPino
Copy link
Contributor

DiegoPino commented Aug 1, 2024

@adam-vessey @glenrobson Test on Ubuntu latest are failing bc docker-compose was replaced by docker compose. Will check on that later.
@adam-vessey also: the JPEG2000 workaround looks great, but wondering, will that not also fail (test are no problem anymore!) with EOF for jpegs and the other metadata classes updated here if corrupt files are encountered in the wild?

@adam-vessey
Copy link
Contributor Author

adam-vessey commented Aug 1, 2024

@DiegoPino:

will that not also fail [...] with EOF for jpegs and the other metadata functions

Yes, but it would've/should've been an issue previously, should whatever underlying stream throw a more general IOException. Kinda looks like the big difference here is that JPEG2000MetadataReaderTest expects SourceFormatException to be thrown (which similar to EOFException is a more specific IOException), while it seems the other metadata readers deal with other IOExceptions when testing failures (GIF has IOException and IllegalStateException in various places, JPEG only has IOException presently). Either way, thanks to polymorphism, more specific IOExceptions being thrown either way should satisfy those other tests which look for IOExceptions.

In terms of the actual problem being fixed, it should no longer enter the infinite loop when encountering the issue with reading described in #659

@DiegoPino
Copy link
Contributor

@adam-vessey thanks for the clarification. That makes sense.

@adam-vessey
Copy link
Contributor Author

adam-vessey commented Aug 13, 2024

Just rebased on top of the latest develop (at d34f259), seeing #682 / #683 go in, which I understand should deal with the build of ubuntu-latest/graalvm to succeed? But now seeing the windows-latest stuff exploding?

Run docker-compose -f docker/Windows-JDK11/docker-compose.yml up --build --exit-code-from cantaloupe
docker-compose: D:\a\_temp\12ceb588-c64f-43d0-bfdc-bb63bdacde5b.ps1:2
Line |
   2 |  docker-compose -f docker/Windows-JDK11/docker-compose.yml up --build  …
     |  ~~~~~~~~~~~~~~
     | The term 'docker-compose' is not recognized as a name of a cmdlet, function, script file, or executable program.
     | Check the spelling of the name, or if a path was included, verify that the path is correct and try again.

Wondering if it might need the same treatment on the Windows side, s/docker-compose/docker compose/? Threw together #685 to give it a whirl.

@DiegoPino
Copy link
Contributor

@adam-vessey yes, just seeing it. Everyday a new thing. Do you want to bring that change into this pull? I can also open another pull just for that and we go from there?( you rebase again?). Also the ubuntu builds/tests sometimes fail because of how the Upload/Cache/S3 tests are build but I can restart them if that happens here.

@adam-vessey
Copy link
Contributor Author

@DiegoPino: I've already got #685 going, just with the docker compose fix. Was thinking to rebase this again after it goes in, but am not opposed to rolling it together, if we want.

@jcoyne
Copy link
Contributor

jcoyne commented Aug 13, 2024

@adam-vessey Would you be able to rebase now?

If they failed to read the number of bytes, the return of `-1` would cause
infinite loops, with `-1` always less than the minimum `offset` of `0`.
@adam-vessey
Copy link
Contributor Author

@jcoyne : Was already in the process of doing so. :P

@adam-vessey
Copy link
Contributor Author

All green, woo!

Copy link
Contributor

@DiegoPino DiegoPino left a comment

Choose a reason for hiding this comment

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

Good! all green

@DiegoPino
Copy link
Contributor

@adam-vessey all good (and green) and approved, thanks so much! I will let others chime in and if nothing comes up I will be merging by tomorrow noon. Have a great day

@DiegoPino DiegoPino merged commit b7acf8b into cantaloupe-project:develop Aug 14, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants