search

hollodotme / fast-cgi-client

A PHP fast CGI client for sending requests (a)synchronously to PHP-FPM
Other
551 stars 34 forks source link

RequestID not unique (Socket ID) #39

Closed BlackBonjour closed 5 years ago

BlackBonjour commented 5 years ago

Expected Behavior

Each request should have an unique ID.

Actual Behavior

Requests get their ID from their socket, which causes that requests do share their IDs. At least when sending requests synchronous.

Specifications

Further comments

For me this is not critical and I am not sure whether I should declare this as a bug or not. Maybe just rename it to socketId or something like that. Normally, all requests should have an unique ID, not matter if the sockets are shared or not. That's not the case right now since the release of 2.7.0.

Currently we are collecting all responses before processing them and to not override something we were using the requestId as I was expecting it to be always unique, which also was the case until the release of 2.7.0.

Right now, we now generate our own unique IDs and ignore the requestId provided by the send... methods and the responses.

hollodotme commented 5 years ago

Hi @BlackBonjour, thank you for the issue.

To clarify: The request ID never was unique since the first version of this library. It actually always was the socket ID and a random interger created when a new socket instance is created (see here). The probability that one gets the same request ID for different requests was always there, but very low.

However in version >= 2.7.0 the socket instances are re-used if possible so that the probability to get the same request ID / socket ID drastically increased.

When I was implementing the re-use of sockets I already noticed that the term request ID really is not appropriate and - I agree with you - it should be called socket ID.

I think I will change it to socket ID in the 3.0.0 release, which is currently in alpha phase.

The question for me is, if it is necessary to introduce a real unique request ID (UUID), because technically the library does not need it to process the requests and, in fact, it could even decrease the performance, if this request ID is returned by Client#sendAsyncRequest() and used to look up or fetch responses for that particular request ID.

Providing a unique request ID in the response only would be easy, but has just informational character. For the library and it functionality it won't have any usage or benefit. But it could have for the user e.g. for logging.

What do you think? What do you use the unique request ID for?

BlackBonjour commented 5 years ago

No, an UUID is not necessary because of the arguments you have written down. If someone really needs it, they can probably extend it by themselfs. Not sure if that would be the cleanest way, but anyway, renaming it to socket ID would do the job.

Our workflow is as followed:

We have multiple adapters which we are calling via FPM (mostly asynchronous) at the same time, but because of various reasons we do not want to process all responses at the same. So to collect the matching responses by our logic, we were fetching them with the request ID (now with the unique ID) from a HashMap provided to the response callback.

Updating to 2.7.0 caused a bug in our software. We did override the FPM responses in our HashMap, because the key was always the request ID. Maybe the bug was always there, but we didn't noticed it until 2.7.0.

We just misused that information.

hollodotme commented 5 years ago

@BlackBonjour Thanks for the explanation. I see the problem.

Thinking about it I'm now considering to remove the request ID / socket ID entirely from the response object as it is just confusing and basically has no beneficial use case other than leading to misusage.

Renaming the the return value of Client#sendAsyncRequest()and the request ID parameters in the other method signatures to socket ID makes sense though.

I think I can do that today in the evening and release a v3.0.0-beta.