I though Roxen might be using it, but maybe I remember it wrong. Anyone else?
I ask because I'm changing Protocols.HTTP.Server.Request to use the Shuffler, and I'm close to done, but it appears that in the process I have had to fix some quite serious bugs in the current shuffler implementation which would/should have prevented it from working correctly in a lot of normal use cases already.
I was then wondering if it is being used successfully anywhere already?
On Thu, Jun 13, 2019 at 9:38 PM Stephen R. van den Berg srb@cuci.nl wrote:
I though Roxen might be using it, but maybe I remember it wrong. Anyone else?
I ask because I'm changing Protocols.HTTP.Server.Request to use the Shuffler, and I'm close to done, but it appears that in the process I have had to fix some quite serious bugs in the current shuffler implementation which would/should have prevented it from working correctly in a lot of normal use cases already.
I was then wondering if it is being used successfully anywhere already?
It's part of the implementation of Process.run(), but any time I make a variant of run(), I end up simplifying it down to not need the shuffler.
ChrisA
I have code that uses the shuffler, and caudium uses it, but I would not make its use mandatory as there are resource implications that probably should be up to the individual user to chose to employ (threading, memory, etc).
Bill
On Jun 13, 2019, at 8:24 AM, Chris Angelico rosuav@gmail.com wrote:
On Thu, Jun 13, 2019 at 9:38 PM Stephen R. van den Berg srb@cuci.nl wrote:
I though Roxen might be using it, but maybe I remember it wrong. Anyone else?
I ask because I'm changing Protocols.HTTP.Server.Request to use the Shuffler, and I'm close to done, but it appears that in the process I have had to fix some quite serious bugs in the current shuffler implementation which would/should have prevented it from working correctly in a lot of normal use cases already.
I was then wondering if it is being used successfully anywhere already?
It's part of the implementation of Process.run(), but any time I make a variant of run(), I end up simplifying it down to not need the shuffler.
ChrisA
H William Welliver III wrote:
I have code that uses the shuffler, and caudium uses it, but I would not make its use mandatory as there are resource implications that probably should be up to the individual user to chose to employ (threading, memory, etc).
The use in HTTP.Server.Request would be optional, hinging on defining a backend to use or not. If you do not specify a backend, it will work as before.
Stephen R. van den Berg wrote:
H William Welliver III wrote:
I have code that uses the shuffler, and caudium uses it, but I would not make its use mandatory as there are resource implications that probably should be up to the individual user to chose to employ (threading, memory, etc).
The use in HTTP.Server.Request would be optional, hinging on defining a backend to use or not. If you do not specify a backend, it will work as before.
On second thought... If you even get to the point that you want to use Protocol.HTTP.Server.Request, you already have a backend running. It does not need extra threading, it uses less memory than without the Shuffler, so what reason would there be not to use it in Protocol.HTTP.Server.Request always?
Well, that’s a major change from long standing functionality. Making a change like that in a wholesale fashion represents a backward compatibility and/or possible stability issue. The shuffler isn’t a free ride (and I’m not sure that your assertions are always true, based on my recollections when we implemented its use in Caudium), so it shouldn’t the default mode of operation.
I also don’t think the fact that a Backend has been specified should be the trigger to use the Shuffler. See my previous comment about backward compatibility and stability.
The request class to use is easily specified when creating a Protocols.HTTP.Server so I don’t see the need to risk breaking code in the wild by modifying the default Request class.
Bill
On Jun 13, 2019, at 12:27 PM, Stephen R. van den Berg srb@cuci.nl wrote:
Stephen R. van den Berg wrote:
H William Welliver III wrote:
I have code that uses the shuffler, and caudium uses it, but I would not make its use mandatory as there are resource implications that probably should be up to the individual user to chose to employ (threading, memory, etc).
The use in HTTP.Server.Request would be optional, hinging on defining a backend to use or not. If you do not specify a backend, it will work as before.
On second thought... If you even get to the point that you want to use Protocol.HTTP.Server.Request, you already have a backend running. It does not need extra threading, it uses less memory than without the Shuffler, so what reason would there be not to use it in Protocol.HTTP.Server.Request always? -- Stephen.
H. William Welliver III wrote:
Making a change like that in a wholesale fashion represents a backward compatibility and/or possible stability issue.
That's why I'm not backporting that change to Pike 8.0.
The shuffler isn???t a free ride (and I???m not sure that your assertions are always true, based on my recollections when we implemented its use in Caudium), so it shouldn???t the default mode of operation.
Well, yes, these assertions were not always true before, but I assumed them to be true, and after committing the necessary fixes to the Shuffler recently, they now *are* true.
I also don???t think the fact that a Backend has been specified should be the trigger to use the Shuffler. See my previous comment about backward compatibility and stability.
If the updated HTTP.Server.Request is not backward compatible, it's a bug and needs to be fixed. AFAICS, it is backward compatible as committed, but I'll happily accept any bugreports.
I implemented a mini-webserver which does on-the-fly compression using a backend running on multiple CPU cores (still have to test if this actually allows me to use significantly more than 100% CPU from Pike).
It works roughly like this:
I'm answering a single GET request, but the answer is the concatenation of an uncompressed file, some pike program uncompressed text output, and a precompressed file:
uncompressed file1 --> ShufflerA --> Gz.Pipe.Compress --------> \ uncompressed program output --> ShufflerB -> Gz.Pipe.Compress -> ---> ShufflerC compressed file2 ---------------------------------------------> /
continued:
--> ShufflerC --> chunked transfer encoding --> socket
The request class to use is easily specified when creating a Protocols.HTTP.Server so I don???t see the need to risk breaking code in the wild by modifying the default Request class.
Because there would be a lot of code duplication and it speeds up and lowers the memory footprint for code using the 8.1 version.
It's not that the Request class is supercomplicated. The code is relatively straightforward, so it should be easy to fix if anything comes up.
It's not that the Request class is supercomplicated. The code is relatively straightforward, so it should be easy to fix if anything comes up.
And only months to years later distributions would supply any user with the fix in a new version of Pike!
While I have not yet had time to look at your changes, on a very general level I agree with Bill, "we can fix it later" is not a good argument; with something as prominent as Protocols.HTTP we should certainly develop the confidence that a change is not going to have negative impact in the field before considering to include it in a release.
Tobias S. Josefowitz @ Pike developers forum wrote:
It's not that the Request class is supercomplicated. The code is relatively straightforward, so it should be easy to fix if anything comes up.
And only months to years later distributions would supply any user with the fix in a new version of Pike!
While I have not yet had time to look at your changes, on a very general level I agree with Bill, "we can fix it later" is not a good argument; with something as prominent as Protocols.HTTP we should certainly develop the confidence that a change is not going to have negative impact in the field before considering to include it in a release.
Ok, point taken. Then again, if anything, the proposed changes to Server.Request simplify the code *a lot*, which (IMO) does count towards making it easier to guarantee correctness (quite possibly even fixing any lingering bugs in the old code).
Less code does not necessarily equate to less bugs, especially since you’re replacing well understood code with code that has not had nearly the same amount of attention.
Just to be clear, I’m not opposed to the idea of a shuffler based server. Were you introducing something new, I would have little reason to object, but this is code that has been in place for 10-15 years (or more) and as such should be subject to appropriate caution, testing and scrutiny before it replaces the default implementation.
In short, subclass Request, implement shuffler there and add it as an option alongside the standard request object. There’s plenty of precedent for that (various backend flavors, various hilfes, etc) and that approach won’t cause any heartburn for anyone.
Bill
Sent from my iPhone
On Jun 14, 2019, at 9:14 AM, Stephen R. van den Berg srb@cuci.nl wrote:
Tobias S. Josefowitz @ Pike developers forum wrote:
It's not that the Request class is supercomplicated. The code is relatively straightforward, so it should be easy to fix if anything comes up.
And only months to years later distributions would supply any user with the fix in a new version of Pike!
While I have not yet had time to look at your changes, on a very general level I agree with Bill, "we can fix it later" is not a good argument; with something as prominent as Protocols.HTTP we should certainly develop the confidence that a change is not going to have negative impact in the field before considering to include it in a release.
Ok, point taken. Then again, if anything, the proposed changes to Server.Request simplify the code *a lot*, which (IMO) does count towards making it easier to guarantee correctness (quite possibly even fixing any lingering bugs in the old code). -- Stephen.
H William Welliver III wrote:
In short, subclass Request, implement shuffler there and add it as an option alongside the standard request object. There???s plenty of precedent for that (various backend flavors, various hilfes, etc) and that approach won???t cause any heartburn for anyone.
Well, I looked at it. Subclassing it is a bit messy. How about this instead: - By default it will use the old buffered send. - If you use set_mode(SHUFFLER) prior to using response_and_finish() it will use the shuffler to send.
That sounds like a reasonable solution to me.
Am sure others will chime in if they disagree with me :)
Bill
Sent from my iPhone
On Jun 18, 2019, at 2:59 AM, Stephen R. van den Berg srb@cuci.nl wrote:
H William Welliver III wrote:
In short, subclass Request, implement shuffler there and add it as an option alongside the standard request object. There???s plenty of precedent for that (various backend flavors, various hilfes, etc) and that approach won???t cause any heartburn for anyone.
Well, I looked at it. Subclassing it is a bit messy. How about this instead:
- By default it will use the old buffered send.
- If you use set_mode(SHUFFLER) prior to using response_and_finish()
it will use the shuffler to send.
Stephen.
Stephen R. van den Berg wrote:
I was then wondering if it is being used successfully anywhere already?
I now know more about the Shuffler code than I ever wanted to know (obviously).
However, armed with that knowledge I am reasonably surprised that anyone managed to use it in the old state without triggering its myriad of lurking segmentation fault and memory corruption pitfalls.
I don't use it myself in Pike 8.0, I use it in Pike 8.1 now. But, in the interest of stabilty I can offer to port the fixes from 8.1 to 8.0 (which, I hope, should be reasonably straightforward, I haven't looked yet).
Should I?
I don't use it myself in Pike 8.0, I use it in Pike 8.1 now. But, in the interest of stabilty I can offer to port the fixes from 8.1 to 8.0 (which, I hope, should be reasonably straightforward, I haven't looked yet).
Should I?
I am still seeing quite a few testsuite failures, like:
Doing tests in modules/_Stdio/testsuite (181 tests, pid 12880) Socket test Child: Copying 19712 bytes of data on 14 fake pipes Failed to read complete data, errno=0, "Success". 0:Input buffer: 0:Expected data: 1536 bytes.
Child failed with errcode 1
Parent: Copying 19712 bytes of data on 14 fake pipes Failed to read complete data, errno=0, "Success". 0:Input buffer: 0:Expected data: 1536 bytes.
3/26 tests failed (skipped 0).
Call me old-school, but I would not necessarily think of backporting to the stable branch while that is going on...
Tobias S. Josefowitz @ Pike developers forum wrote:
I am still seeing quite a few testsuite failures, like:
Doing tests in modules/_Stdio/testsuite (181 tests, pid 12880) Socket test Child: Copying 19712 bytes of data on 14 fake pipes Failed to read complete data, errno=0, "Success". 0:Input buffer: 0:Expected data: 1536 bytes.
Those are probably related to me or someone else rebreaking the FakePipe. I'll check it.
Call me old-school, but I would not necessarily think of backporting to the stable branch while that is going on...
:-). Well, I don't think these problems touch the Shuffler.
Tobias S. Josefowitz @ Pike developers forum wrote:
:-). Well, I don't think these problems touch the Shuffler.
Oh I see, I thought FakePike would be using Shuffler...
No, indeed.
I fixed the FakePipe issue. It passes all tests, AFAICS.
With regard to the Shuffler: the Pike 8.0 shuffler is very fragile and could (at any time) result in datacorruption and/or segmentation faults. Even though, considering the incremental discovery of all the errors in the Shuffler feels a bit like whack-a-mole, I can reason that most likely the grave memory corruption bugs are gone, considering that all callback connections between Shuffle and sources have now been covered and rewritten.
Stephen R. van den Berg wrote:
With regard to the Shuffler: the Pike 8.0 shuffler is very fragile and could (at any time) result in datacorruption and/or segmentation faults. Even though, considering the incremental discovery of all the errors in the Shuffler feels a bit like whack-a-mole, I can reason that most likely the grave memory corruption bugs are gone, considering that all callback connections between Shuffle and sources have now been covered and rewritten.
I checked the 8.0 version of the Shuffler. There are some differences, not sure if they present a problem backporting. The simplest would probably be to replace the whole 8.0 version of the module with the 8.1 version; the question is if that would be a viable option (or even works without too much trouble).
I personally have no need for a working Shuffler in 8.0, so I'll leave it untouched.
pike-devel@lists.lysator.liu.se
-
Chris Angelico -
H William Welliver III -
H. William Welliver III
-
Stephen R. van den Berg -
Tobias S. Josefowitz ＠ Pike developers forum