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

Error handling for FileAccess.get_file_as_* #82595

Merged
merged 1 commit into from
Oct 11, 2023

Conversation

paulloz
Copy link
Member

@paulloz paulloz commented Sep 30, 2023

What happens on errors in FileAccess.get_file_as_bytes and FileAccess.get_file_as_string is currently undocumented. It's also impossible to retrieve the actual error that happened.


  • Assign last error in said FileAccess.get_file_as_bytes and FileAccess.get_file_as_string
  • Document error handling for said methods

@paulloz paulloz requested review from a team as code owners September 30, 2023 22:20
@paulloz paulloz force-pushed the fix-fileaccess-error-handling branch from 02fbf74 to aac7be9 Compare September 30, 2023 22:31
@Chaosus Chaosus added this to the 4.2 milestone Oct 2, 2023
@RandomShaper
Copy link
Member

As far as I can see, last_file_open_error should only be set in the case of an failed open. In that line of thought, get_file_as_bytes could just set it if the opening fails:

	Error open_error = OK;
	Ref<FileAccess> f = FileAccess::open(p_path, READ, open_error);
	if (f.is_null()) {
		last_file_open_error = open_error;
		if (r_error) { // if error requested, do not throw error
			*r_error = open_error;
			return Vector<uint8_t>();
		}
		ERR_FAIL_V_MSG(Vector<uint8_t>(), "Can't open file from path '" + String(p_path) + "'.");
	}

What do you think?

@akien-mga akien-mga requested a review from bruvzg October 2, 2023 10:15
@paulloz
Copy link
Member Author

paulloz commented Oct 2, 2023

@RandomShaper That was my initial line of thought too (with the last_file_open_error assignation above the if, to always assign it). In the end, I wasn't sure what was best, but I'm fine with any.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@akien-mga
Copy link
Member

akien-mga commented Oct 11, 2023

As far as I can see, last_file_open_error should only be set in the case of an failed open.

I had missed this when reviewing, but I don't think that's correct with the current implementation.

Ref<FileAccess> FileAccess::_open(const String &p_path, ModeFlags p_mode_flags) {
	Error err = OK;
	Ref<FileAccess> fa = open(p_path, p_mode_flags, &err);
	last_file_open_error = err;
	if (err) {
		return Ref<FileAccess>();
	}
	return fa;
}

Ref<FileAccess> FileAccess::open_encrypted(const String &p_path, ModeFlags p_mode_flags, const Vector<uint8_t> &p_key) {
	Ref<FileAccess> fa = _open(p_path, p_mode_flags);
	if (fa.is_null()) {
		return fa;
	}

	Ref<FileAccessEncrypted> fae;
	fae.instantiate();
	Error err = fae->open_and_parse(fa, p_key, (p_mode_flags == WRITE) ? FileAccessEncrypted::MODE_WRITE_AES256 : FileAccessEncrypted::MODE_READ);
	last_file_open_error = err;
	if (err) {
		return Ref<FileAccess>();
	}
	return fae;
}

last_file_open_error is always set, whether it succeeds or fails.

The name might be a bit confusing, but it's the last Error value (which includes OK), not the last time there was an "error" (failure).

It might be worth clarifying this in the documentation for FileAccess and DirAccess get_open_error.

@paulloz paulloz force-pushed the fix-fileaccess-error-handling branch from aac7be9 to 54ea062 Compare October 11, 2023 16:25
@akien-mga
Copy link
Member

The CI error is a GitHub Actions fluke. Rebasing on top of the just merged #83147 might help (though not 100% sure yet).

- Assign last error in said `FileAccess.get_file_as_bytes` and `FileAccess.get_file_as_string`
- Document error handling for said methods
@paulloz paulloz force-pushed the fix-fileaccess-error-handling branch from 54ea062 to bf3f6e3 Compare October 11, 2023 16:52
@akien-mga akien-mga merged commit 587f084 into godotengine:master Oct 11, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@paulloz paulloz deleted the fix-fileaccess-error-handling branch October 11, 2023 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants