Set curl CURLOPT_TIMEOUT to prevent processes being stuck after connecting to server, introduce HTTP transports

[FelixJacobi]

E.g. connection was successful but server does not respond properly.

We encountered the situation that the HTTP connection was initially established but the cron script using the library hangs afterwards for an infinity time (something was wrong on TCP level). Debugging the issue showed something was wrong with the network link between both machines (fault on side of the ISP). This change adds a reliable timeout for the whole request process in cURL to prevent the library from hanging forever in future if such situation occurs.

I submitted this patch already against original upstream in https://github.com/bigbluebutton/bigbluebutton-api-php/pull/120. But it seems that this fork has more activity and the PR more likely will be accepted in near future.

Thank you for your time.

[sualko]

Thanks for your pr. I think it's a great enhancement, but maybe 20 seconds are too short. How about 30 seconds?

[SamuelWei]

Thanks for your pr. I think it's a great enhancement, but maybe 20 seconds are too short. How about 30 seconds?

Maybe we could have the time as a parameter the users can adjust?

[sualko]

Maybe we could have the time as a parameter the users can adjust?

We could add it as parameters to the BigBlueButton, or use some static variables, so we don't have to pass it down. I'm wondering if there is no standard way of configuring curl via some php parameters.

[joisarjignesh]

Maybe we could have the time as a parameter the users can adjust?

We could add it as parameters to the BigBlueButton, or use some static variables, so we don't have to pass it down. I'm wondering if there is no standard way of configuring curl via some PHP parameters.

yes I think we need to set up the third parameter as configuration on an array (optional) and also we need to set the default curl configuration array value

[FelixJacobi]

Thanks for your pr. I think it's a great enhancement, but maybe 20 seconds are too short. How about 30 seconds?

Would be fine for me, I guess. The initial approach was more like a qnd fix to repair our infrastructure ;-) .

Maybe we could have the time as a parameter the users can adjust?

We could add it as parameters to the BigBlueButton, or use some static variables, so we don't have to pass it down. I'm wondering if there is no standard way of configuring curl via some php parameters.

yes I think we need to set up the third parameter as configuration on an array (optional) and also we need to set the default curl configuration array value

Yeah, sounds reasonable. But I am not really sure if this should belong into the BigBlueButton class directly. I would possibly pursue the idea of moving the direct use of cURL out of this class into a separate class for a transport (maybe with a suitable interface).

The values can then be injected via the constructor of this class instead of BigBlueButton and a suitable configured instance can be passed if necessary. It would further allow to replace the cURL implementation e.g. with a PSR HTTP client or similar if one e.g. wants to perform integration tests with the code.

If I am correct, it would be possible even without introducing any BC breaks.

[sualko]

Maybe we can combine all ideas:

• increase the timeout to a usable value
• allow to inject transport class
• add params to curl transport

We just need someone who is willing to implement this :smile: Volunteers?

[FelixJacobi]

We just need someone who is willing to implement this smile Volunteers?

I can work on it this weekend.

[FelixJacobi]

Hi everyone,

A first draft of HTTP transports is now included in this PR. I moved the original cURL integration into a CurlTransport. I also added a TransportInterface and transports for Symfony HTTP Client and PSR18 HTTP Client as quite common and spread libraries (possibly could be moved into some separate Composer packages later).

Furthermore, I am planning further refinements (e.g. add tests) during the next days. The code was also not yet tested in an environment with a real BBB server. Comments already welcome.

[FelixJacobi]

I also addresses the suggestion by @sualko and raised the default request timeout in CurlTransport to 30 seconds.

[FelixJacobi]

From my side, this PR should be finished now. I incorporated some further minor improvements and included documentation and tests (even for the CurlTransport with a tricky but good working solution).

[sualko]

From my side, this PR should be finished now. I incorporated some further minor improvements and included documentation and tests (even for the CurlTransport with a tricky but good working solution).

Great. Do you have time to merge the master branch, before we do a review?

[FelixJacobi]

From my side, this PR should be finished now. I incorporated some further minor improvements and included documentation and tests (even for the CurlTransport with a tricky but good working solution).

Great. Do you have time to merge the master branch, before we do a review?

Sure. Will do it this evening or tomorrow.

[FelixJacobi]

From my side, this PR should be finished now. I incorporated some further minor improvements and included documentation and tests (even for the CurlTransport with a tricky but good working solution).

Great. Do you have time to merge the master branch, before we do a review?

@sualko Done.

Edit: Broken rebase, I am currently fixing this mess ^^ .

[FelixJacobi]

I noticed that the PHP 7.2 pipeline seems to permanently fail. The failure seems to be related to Coveralls. Anyone knows what to do?

[SamuelWei]

I noticed that the PHP 7.2 pipeline seems to permanently fail. The failure seems to be related to Coveralls. Anyone knows what to do?

https://github.com/littleredbutton/bigbluebutton-api-php/pull/60/commits/e86e8a211eaba66acdc3d799c4011503e565f3c2

[FelixJacobi]

I noticed that the PHP 7.2 pipeline seems to permanently fail. The failure seems to be related to Coveralls. Anyone knows what to do?

e86e8a2

Hm, seems already to be applied in this branch.

[FelixJacobi]

Regarding #64: Would it help if someone with write permissions to this repository made a copy of the branch and create a new PR?

[sualko]

Should be no big issue to merge this pr from command line, but until now I had no time to look at it. Sorry.

[FelixJacobi]

Should be no big issue to merge this pr from command line, but until now I had no time to look at it. Sorry.

That's fine ^^ . I was not really sure how to move forward here and when I saw the issue yesterday I thought it was probably blocked because of that. Just asking.

Thanks for the update so far.

[sualko]

@SamuelWei can you give a time frame until you have reviewed this? I think after this is merged we can release v3.

[SamuelWei]

@SamuelWei can you give a time frame until you have reviewed this? I think after this is merged we can release v3.

I agree, that would be a big improvement and change that we should release as v3.4.0

Giving a timeframe isn't easy, as my daily schedule gets changed quite frequently. I hope I'll be able to have a first deeper look into the PR tomorrow and next week. Today I just scrolled through all the changes and liked the idea and implementation.

[FelixJacobi]

Should be ready now. I added integration tests for the new transports like there already exists for the original cURL one.

[sualko]

I think we can merge this now. If we find bugs we can fix them afterwards. Before I release v3, I want to test this in my application.

@FelixJacobi thanks for this awesome job.

[sualko]

Correction: We have to release v4, since we are already on 3.x :see_no_evil:.

@FelixJacobi if you like you can create another pr to add yourself to the list of authors in the composer file.

[FelixJacobi]

@FelixJacobi if you like you can create another pr to add yourself to the list of authors in the composer file.

Sure, thanks. Will do this in #67.