replace guzzle with amphp/http-client and revolt/event-loop #500
Conversation
unglaublicherdude
force-pushed
the
replace-guzzle-with-react-http
branch
from
f158302 to
42c969c
Compare
|
The order matters, when using the react/event-loop because one http-request can consume the readstream that accidental also in the loop. See this issue for that. For a better understanding I added this test What you see is, that with the second http request in a second browser the first stream is getting consumed even though the stream is completely unrelated to the second browser. |
unglaublicherdude
force-pushed
the
replace-guzzle-with-react-http
branch
2 times, most recently
from
3a6a9af to
5a713cf
Compare
|
I also opened this discussion here. |
|
I also opened this issue. |
unglaublicherdude
changed the title
unglaublicherdude
requested review from
pstadermann,
GermanCoding,
doxthree and
lennartdohmann
|
I completely replaced react with amphp and revolt which works as expected. Streams are not getting touched before something is actually consuming them. |
|
Dunno why it did not start the pipeline. Here is manual run of these changes. |
| $verdictResponse = $this->_verdictResponseForStream( | ||
| $uuid | ||
| ); | ||
| $this->_logger->debug("uuid: ".var_export($uuid, true)); |
There was a problem hiding this comment.
Considering that uuid is typed to be a string, isn't the var_export a bit too much here? A simple . $uuid should suffice I think.
| ); | ||
| $this->_logger->debug("uuid: ".var_export($uuid, true)); | ||
| $uuid = $uuid !== null ? $uuid : UuidV4::getFactory()->uuid4()->toString(); | ||
| $this->_logger->debug("uuid: ".var_export($uuid, true)); |
There was a problem hiding this comment.
Double debug print looks like this is a leftover from debugging. A single print in this line is enough debug logging IMHO.
There was a problem hiding this comment.
Both where debugging stuff before I found out, that in VSCode you just have to start the debugger first, then you will basically debug anything you run in php like starting the test from the testexplorer but also from console. That made my life so much easier.
|
|
||
| $this->assertEquals(Verdict::MALICIOUS, $verdict->Verdict); | ||
| $this->assertEquals("text/plain", $verdict->MimeType); | ||
| $this->assertNotEmpty($verdict->Detection); | ||
| } | ||
|
|
||
| static function random_strings($length_of_string) { |
There was a problem hiding this comment.
There is a duplicate function here.
vaas/php/tests/vaas/StreamsInLoopTest.php
Line 29 in 48121e0
There was a problem hiding this comment.
Ah yes. I had problems calling stuff from another class and just copied it here. And in the end, I don't need it anymore. Will remove it.
… don't spam the log. Also unify logging by extracting it to a wrapper-function.
…consumed in the loop by pausing and resuming it
It does not run, when no branch filter matches This reverts commit d76bed6.
Co-authored-by: Max <[email protected]>
Co-authored-by: Max <[email protected]>
Co-authored-by: Max <[email protected]>
Co-authored-by: doxthree <[email protected]>
unglaublicherdude
force-pushed
the
replace-guzzle-with-react-http
branch
from
68be70d to
3c717c5
Compare
php/tests/vaas/VaasTest.php
Outdated
|
|
||
| $verdict = $vaas->ForStream($stream); | ||
| /** | ||
| * @throws GuzzleException |
There was a problem hiding this comment.
This seems corrupted. Guzzle is completely removed (?)
No description provided.