"The responsibility for the stream goes into phpcr who must close it"

[Christian Stocker]

Hi

I'm trying to improve jackalope-jackrabbit performance with using JSOP
which makes saving all changes with one http call possible. See
https://wiki.apache.org/jackrabbit/Jsop for the protocol.

While I'm through with all the important stuff (see
https://github.com/chregu/jackalope-jackrabbit/tree/jsop for the work in
progress), I'm fighting with some last failing tests. One I'm not sure,
what it tries to cover. It's about streams and the following message

"The responsibility for the stream goes into phpcr who must close it"
(see below for the full output)

for some reason, current master indeed does close those streams somehow,
but I can't figure out where. But OTOH, if I do the following:

***
$f = $rootNode->getNode("foo");
$stream = fopen('php://memory', 'w+');
fwrite($stream, 'foo bar');
rewind($stream);
$bin = $f->setProperty('newBinaryStream', $stream,
\PHPCR\PropertyType::BINARY);
$session->logout();
***

$stream is still a stream after the logout

So. What does this code try to cover? and why?

Then I can maybe fix it ;)

Greetings

chregu

***
phpunit test output
***
jackalope-jackrabbit/tests$ phpunit
./phpcr-api/tests/10_Writing/SetPropertyTypesTest.php
PHPUnit 3.6.10 by Sebastian Bergmann.

Configuration read from /opt/git/jackalope-jackrabbit/tests/phpunit.xml.dist

..FF................

Time: 3 seconds, Memory: 13.25Mb

There were 2 failures:

1) PHPCR\Tests\Writing\SetPropertyTypesTest::testCreateValueBinaryFromStream
The responsibility for the stream goes into phpcr who must close it
Failed asserting that true is false.

/opt/git/jackalope-jackrabbit/tests/phpcr-api/tests/10_Writing/SetPropertyTypesTest.php:68
/usr/local/php5-20120203-085139/bin/phpunit:46

2)
PHPCR\Tests\Writing\SetPropertyTypesTest::testCreateValueBinaryFromStreamAndRead
The responsibility for the stream goes into phpcr who must close it
Failed asserting that true is false.

/opt/git/jackalope-jackrabbit/tests/phpcr-api/tests/10_Writing/SetPropertyTypesTest.php:88
/usr/local/php5-20120203-085139/bin/phpunit:46

FAILURES!
Tests: 20, Assertions: 90, Failures: 2.
***
--
Liip AG // Feldstrasse 133 // CH-8004 Zurich
Tel +41 43 500 39 81 // Mobile +41 76 561 88 60
www.liip.ch // blog.liip.ch // GnuPG 0x0748D5FE

[Lukas Kahwe Smith]

On Mar 4, 2012, at 12:13 , Christian Stocker wrote:

> Hi
>
> I'm trying to improve jackalope-jackrabbit performance with using JSOP
> which makes saving all changes with one http call possible. See
> https://wiki.apache.org/jackrabbit/Jsop for the protocol.
>
> While I'm through with all the important stuff (see
> https://github.com/chregu/jackalope-jackrabbit/tree/jsop for the work in
> progress), I'm fighting with some last failing tests. One I'm not sure,
> what it tries to cover. It's about streams and the following message
>
> "The responsibility for the stream goes into phpcr who must close it"
> (see below for the full output)
>
> for some reason, current master indeed does close those streams somehow,
> but I can't figure out where. But OTOH, if I do the following:

its done in Property::__destruct()

https://github.com/jackalope/jackalope/blob/master/src/Jackalope/Property.php#L692

regards,
Lukas Kahwe Smith
sm...@pooteeweet.org

[Christian Stocker]

I saw that, but the code never goes there before the end of the script,
because $session->logout() doesn't unset the Properties (afaics).
$session->logout doesn't do much anyway and I guess it happens in
"$this->saveAndRenewSession();" in tests/phpcr-api/inc/BaseCase.php
somewhere which makes the test pass. But I'm not sure, which real case
scenario works like this testcase.

David, you wrote that test, do you know remember, why and what was the
intention, according to the log

commit 760f567d3840dc70dd2863dd09e1f21cd8161df9
Author: dbu <david.b...@liip.ch>
Date: Tue Nov 29 19:16:33 2011 +0100

adding tests about closing binary streams when writing

But the question is, when exactly should the binary stream be closed? on
object destruction (which is then handeld automaticaly by PHP somehow
via the __destructor) or earlier? Eg. $session->logout?

Greetings

chregu

>
> regards,
> Lukas Kahwe Smith
> sm...@pooteeweet.org
>
>

--

[Lukas Kahwe Smith]

yeah .. seems to make sense that we close them on logout. and we should do an implicit logout on destruct in the session.

[Christian Stocker]

But we don't unset properties or nodes, afaics. should we get rid of the
whole objects and objectmanager on session->logout()?

I guess so :)

chregu

>
> regards,
> Lukas Kahwe Smith
> sm...@pooteeweet.org
>
>

--

[David Buchmann]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

hi chregu,

the close operation that makes the test successful is here:

https://github.com/jackalope/jackalope-jackrabbit/blob/master/src/Jackalope/Transport/Jackrabbit/Client.php#L973

the point is that either the save operation or the logout should lead to
the stream being closed, the user should not need to keep track of it if
he needs it to be closed when no longer needed.

now you are more the experts on streams than i am, so if you consider
this a stupid idea, we can remove the test. but i am fearing confusion
and waste of resources - and that people might use the streams they put
into jackalope elsewhere, with probably not the expected results.

cheers,david

- --
Liip AG // Agile Web Development // T +41 26 422 25 11
CH-1700 Fribourg // PGP 0xA581808B // www.liip.ch
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk9U3nMACgkQqBnXnqWBgIv8MwCeN6QrTF3CtWY/JOXFoHItclXA
mccAoJB9sZluszZ3TNaNa8fSbD+EqFZ+
=upcW
-----END PGP SIGNATURE-----

[Christian Stocker]

Hi

Oh yes, that helped. I didn't port that fclose ;)
Now those 2 tests pass

Not totally sure, if it's really needed, but better be safe here is a
good idea.

Thanks a lot for the response

chregu

--