Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Drop free_filename field from zend_file_handle
free_filename was always zero.
- Loading branch information
Showing
7 changed files
with
0 additions
and
17 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
e0eca26
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nikic This should be documented in UPGRADING.INTERNALS
@petk @EricSten-MSFT This is the commit that breaks Wincache in 7.4-beta1
e0eca26
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nikic See php/pecl-caching-wincache@def79d4#commitcomment-34413735
e0eca26
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WinCache is
estrdup()
'ing a filename into thezend_file_handle
during its stream open, and we're assuming that whomever free's the handle will need to do theefree()
on that memory. Is there some other way to signal that whomever frees the file handle needs to free the filename pointer?Is there a
PHP_API_VERSION
version number change to go with this breaking change? I see that 20190529 is the current value in the main/php.h file, but that was the same number as php-7.4.0alpha1. Otherwise I won't be able to compile for both before and after php-7.4.0beta1.e0eca26
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@EricSten-MSFT As long as you set opened_path (which I believe you do), just setting filename to NULL (and thus no need to estrdup anything) should be fine.
e0eca26
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this turns out to be too troublesome it would be possible to add
free_filename
back (I wasn't aware it's used by wincache). I'd just like to explore not having it first, because it seems like an unnecessary complication on both sides.e0eca26
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The memory for the string used by
opened_path
is actually the allocation fromfilename
, so one way or the other, when thezend_file_handle
gets freed, the memory needs to get free'd. I don't know how many other extensions are implementing their own stream open handler and returning their ownzend_file_handle
s, but they must run into the same problem, since they don't own the code that does the free'ing of the returned object.So, I'm going to say, yeah, it will be too troublesome to unwind this. I would greatly appreciate your patience, and hope that you will revert this change. Thx!
e0eca26
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose, wincache should work in the same way as opcache, so it probably may be fixed, or both should have the same problem.
e0eca26
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like opcache always deals with interned strings, so it never has to worry about cleaning up the memory. However, WinCache (for reasons before my time) has its own resolve cache, and every time a stream_open happens, it allocates the filename based upon whatever is returned from the resolve cache. It therefore needs whomever free's up the
zend_file_handle
to also free up the filename.e0eca26
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll look into this tomorrow and either come up with a way to make wincache work without free_filename, or revert the change.
e0eca26
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the changes in the commit, there are almost no benefits by the removal. And for extensions that use it the hurdles are high. I agree with @EricSten-MSFT that a revert would be best. If it is reverted, please do a new release of beta1 before the official announcement.
e0eca26
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change reverted in d968027.
e0eca26
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Will you make a new release tarball? The one at github is still from yesterday.
e0eca26
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @derickr (the tarballs are otherwise planned for the Thursday to be "final", exactly for cases like this).
e0eca26
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll see if I can roll some new ones right away.
@EricSten-MSFT — we'll bump the API number with RC1. Before that, the API doesn't have to be stable yet.
e0eca26
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! → https://gist.github.com/derickr/6062a94c900289397130deb562ed17e1 for http://downloads.php.net/~derick/
e0eca26
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was fast. Thank you @derickr
e0eca26
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd already noticed. Compiling the new beta1 nts x64 vs16 ATM.
e0eca26
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix confirmed. Thanks to all.