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

Raise on File.stream's aenter if the file is a dir or doesn't exist #1046

Merged
merged 10 commits into from
Mar 3, 2022

Conversation

FasterSpeeding
Copy link
Collaborator

@FasterSpeeding FasterSpeeding commented Mar 2, 2022

Summary

This matches the behaviour seen for URL resources (which happen to be the only other kind where the resource may not exist or may be invalid) and should solve the issue of aiohttp giving off confusing errors on unknown files by erroring before we enter an aiohttp context.

Checklist

  • I have run nox and all the pipelines have passed.
  • I have made unittests according to the code I have added/modified/deleted.

Related issues

Fixes: #826

@FasterSpeeding FasterSpeeding enabled auto-merge (squash) March 2, 2022 08:08
Copy link
Member

@davfsa davfsa left a comment

Choose a reason for hiding this comment

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

I am not overly happy with this. It does provide a more helpful error, but if people now how to debug python, they should be able to notice the file not found error above the error to send the request.

Additionally, this doesnt guarantee that the file is not deleted or moved after it is opened, which can lead to even more errors.

I would rather just have aiohttp deal with this and not us.

hikari/files.py Show resolved Hide resolved
@FasterSpeeding
Copy link
Collaborator Author

FasterSpeeding commented Mar 2, 2022

I am not overly happy with this. It does provide a more helpful error, but if people now how to debug python, they should be able to notice the file not found error above the error to send the request.

Ok but this just matches the behaviour for URL where if it doesn't exist it'll error before being "read" into, giving a guarantee t that file.__aenter__ will error if a resource doesn't exist instead of making that dependent on whether its a URL or File

Additionally, this doesnt guarantee that the file is not deleted or moved after it is opened, which can lead to even more errors.

It's OS impl detail on whether that even matters anyways: unix systems will keep the file alive until the last application closes it and windows just doesn't let you delete a file if its open in another app; what we might be able to do to handle this is keep the file pointer alive instead of re-opening it once they start reading so all that'd really need to be changed is to keep it open until its finished reading it rather than re-opening it which should be an easy change for threaded file reads at least but idk about process pool file reads so

@FasterSpeeding
Copy link
Collaborator Author

FasterSpeeding commented Mar 3, 2022

I actually have an impl which keeps the file open for the duration of the context manager for thread based file readers locally already but I don't think the same behaviour can be achieved with a process file reader (well unless you just do something hacky that also opens the file in a thread pool)

Im also not sure what the point of allowing a process pool be used for file reading is (like file reading is blocking IO, not cpu bound work). Kinda seems like unnecessary overhead and complexity unless I'm missing something

@davfsa
Copy link
Member

davfsa commented Mar 3, 2022

If you think it is worth having, then we can go for it

hikari/files.py Show resolved Hide resolved
@FasterSpeeding FasterSpeeding merged commit 7ba52c6 into hikari-py:master Mar 3, 2022
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.

Better handling of non-existent attachment attachment paths
2 participants