[release/1.3 backport] Bump Golang 1.13.10

backport of https://github.com/containerd/containerd/pull/4145 and https://github.com/containerd/containerd/pull/4169
go1.13.10 (released 2020/04/08) includes fixes to the go command, the runtime,
os/exec, and time packages. See the Go 1.13.10 milestone on the issue tracker
for details:
https://github.com/golang/go/issues?q=milestone%3AGo1.13.10+label%3ACherryPickApproved
full diff: https://github.com/golang/go/compare/go1.13.9...go1.13.10

Codecov Report

Merging #4170 into release/1.3 will increase coverage by 3.49%.
The diff coverage is n/a.

@@               Coverage Diff               @@
##           release/1.3    #4170      +/-   ##
===============================================
+ Coverage        42.43%   45.93%   +3.49%
===============================================
  Files              130      117      -13
  Lines            14364    11499    -2865
===============================================
- Hits              6096     5282     -814
+ Misses            7373     5323    -2050
+ Partials           895      894       -1

Flag
Coverage Δ

#linux
45.93% <ø> (ø)

#windows
?

Impacted Files
Coverage Δ

remotes/docker/fetcher.go
54.11% <0.00%> (-5.41%)
:arrow_down:

remotes/docker/auth.go
63.82% <0.00%> (-3.97%)
:arrow_down:

remotes/docker/status.go
21.42% <0.00%> (-3.58%)
:arrow_down:

filters/scanner.go
81.35% <0.00%> (-0.85%)
:arrow_down:

platforms/cpuinfo.go
3.77% <0.00%> (-0.85%)
:arrow_down:

platforms/platforms.go
79.41% <0.00%> (-0.81%)
:arrow_down:

log/context.go
36.84% <0.00%> (-0.66%)
:arrow_down:

remotes/docker/handler.go
22.38% <0.00%> (-0.09%)
:arrow_down:

errdefs/grpc.go
77.46% <0.00%> (-0.04%)
:arrow_down:

content/content.go
0.00% <0.00%> (ø)

... and 73 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13ac9e7...ec5e25d. Read the comment docs.


Suggested Topic: Regular Expressions

@jsstrn Did you have a really good tutorial for RegEx?

@cbas Yes, as a matter of fact, I do: http://regexone.com/
On Monday, 4 January 2016, Sebastiaan Deckers<EMAIL_ADDRESS>wrote:

@jsstrn https://github.com/jsstrn Did you have a really good tutorial
for RegEx?
—
Reply to this email directly or view it on GitHub
https://github.com/ga-students/wdi-sg-organisers/issues/7.

:+1: Thanks for the link!

Use case demo: http://codepen.io/cbas/pen/NxdZpm

Anti-use case: http://stackoverflow.com/questions/1732348/regex-match-open-tags-except-xhtml-self-contained-tags

Other stuff shared during the lesson:

/((\+|00)?65[\.\-\s]?)?\d{4}[\.\-\s]?\d{4}/ regex to match phone numbers in SG
https://regexcrossword.com/
https://regex101.com/

Thanks for the help @jsstrn :+1:


Server crash when a convertor in a chain crashes

When the second convertor in a chain crashes, the entire server crashes.
This PR adds a test case.

First I wonder if the test being done here is actually testing what needs to be tested. The CrashingConverter throw an error in the data listener, but this is not how a converter should throw an error. The converter is not crashing here, the callback function is going to crash which would indicate that we're working with a flawed converter design. The correct way would be to emit an error on data if something goes wrong, which actually causes the fetch to fail because of a socket hang up. This is expected behaviour because the headers have already been sent before the error is detected so it's impossible to send a 500 at that point which is why we just break off the connection.
I'm actually surprised there is no crash when doing the suggested change in the config which might indicate I'm making the wrong assumptions above 😄 . The logger error does make sense though. Nobody is listening to the error of the representation.data stream since that would be the responsiblity of the CrashingConverter in this case. If you add an error listener to that stream in the handle function that logger error disappears.

First I wonder if the test being done here is actually testing what needs to be tested. The CrashingConverter throw an error in the data listener, but this is not how a converter should throw an error. The converter is not crashing here, the callback function is going to crash which would indicate that we're working with a flawed converter design. The correct way would be to emit an error on data if something goes wrong, which actually causes the fetch to fail because of a socket hang up. This is expected behaviour because the headers have already been sent before the error is detected so it's impossible to send a 500 at that point which is why we just break off the connection.
I'm actually surprised there is no crash when doing the suggested change in the config which might indicate I'm making the wrong assumptions above 😄 . The logger error does make sense though. Nobody is listening to the error of the representation.data stream since that would be the responsiblity of the CrashingConverter in this case. If you add an error listener to that stream in the handle function that logger error disappears.

First I wonder if the test being done here is actually testing what needs to be tested. The CrashingConverter throw an error in the data listener

Yes, we are testing a misbehaving component indeed.
It should not take down the entire server.

, but this is not how a converter should throw an error.

I've done it in a quite obvious way now, but I had this actually happening a few calls down the line. It would mean that we'd need to add a try/catch to all data handlers; and we can't enforce that on externally developed components (which is where I had it happen).

The correct way would be to emit an error on data if something goes wrong

I believe this is actually what happens: if a listener crashes, the stream emits the error.

The logger error does make sense though. Nobody is listening to the error of the representation.data stream since that would be the responsiblity of the CrashingConverter in this case.

Agreed.

If you add an error listener to that stream in the handle function that logger error disappears.

So we are in the case where the handler misbehaves—somebody down the lines needs to catch it.

First I wonder if the test being done here is actually testing what needs to be tested. The CrashingConverter throw an error in the data listener

Yes, we are testing a misbehaving component indeed.
It should not take down the entire server.

, but this is not how a converter should throw an error.

I've done it in a quite obvious way now, but I had this actually happening a few calls down the line. It would mean that we'd need to add a try/catch to all data handlers; and we can't enforce that on externally developed components (which is where I had it happen).

The correct way would be to emit an error on data if something goes wrong

I believe this is actually what happens: if a listener crashes, the stream emits the error.

The logger error does make sense though. Nobody is listening to the error of the representation.data stream since that would be the responsiblity of the CrashingConverter in this case.

Agreed.

If you add an error listener to that stream in the handle function that logger error disappears.

So we are in the case where the handler misbehaves—somebody down the lines needs to catch it.

Yes, we are testing a misbehaving component indeed.
It should not take down the entire server.

This is related to #314, there's only so much we can do unless we take a drastic solution as mentioned there.

I believe this is actually what happens: if a listener crashes, the stream emits the error.

Going to have another look then why this specifically doesn't do what is expected. If that is indeed just throwing an error in the original stream than there might be something wrong (which might be the case seeing the second part of my comment). But I don't think we should account for all possible things that might be wrongly implemented in external modules. A very extreme case might be a module calling process.exit() and in that case I think we should just suggest not using that module 😄

If you add an error listener to that stream in the handle function that logger error disappears.

So we are in the case where the handler misbehaves—somebody down the lines needs to catch it.

The point of that logger message is to warn us that there is something wrong with the implementation though and is an indication that we should fix our modules. This is us catching it already in the guarded stream. If there was no guarded stream there we would have a server crash instead of just a logger message.

Yes, we are testing a misbehaving component indeed.
It should not take down the entire server.

This is related to #314, there's only so much we can do unless we take a drastic solution as mentioned there.

I believe this is actually what happens: if a listener crashes, the stream emits the error.

Going to have another look then why this specifically doesn't do what is expected. If that is indeed just throwing an error in the original stream than there might be something wrong (which might be the case seeing the second part of my comment). But I don't think we should account for all possible things that might be wrongly implemented in external modules. A very extreme case might be a module calling process.exit() and in that case I think we should just suggest not using that module 😄

If you add an error listener to that stream in the handle function that logger error disappears.

So we are in the case where the handler misbehaves—somebody down the lines needs to catch it.

The point of that logger message is to warn us that there is something wrong with the implementation though and is an indication that we should fix our modules. This is us catching it already in the guarded stream. If there was no guarded stream there we would have a server crash instead of just a logger message.

Crashing harder now with efdab1ca8d4ca303dc9dbba35fdb92f341eccbf9.

Crashing harder now with efdab1ca8d4ca303dc9dbba35fdb92f341eccbf9.

The point of that logger message is to warn us that there is something wrong with the implementation though and is an indication that we should fix our modules. This is us catching it already in the guarded stream. If there was no guarded stream there we would have a server crash instead of just a logger message.

Agreed—the system is telling us to add an error listener. Hence this PR, where shall we add it? 😉

The point of that logger message is to warn us that there is something wrong with the implementation though and is an indication that we should fix our modules. This is us catching it already in the guarded stream. If there was no guarded stream there we would have a server crash instead of just a logger message.

Agreed—the system is telling us to add an error listener. Hence this PR, where shall we add it? 😉

Nobody is listening to representation.data it seems. Someone really should. Not the convertors themselves.

I guess this is the part I disagree with 😄. I do think that something that implements RepresentationConverter has the responsibility of handling that data, either by destroying it or listening to it. And if they don't we should get that logger error telling us that there is a faulty implementation (it shouldn't crash the entire server of course).

Nobody is listening to representation.data it seems. Someone really should. Not the convertors themselves.

I guess this is the part I disagree with 😄. I do think that something that implements RepresentationConverter has the responsibility of handling that data, either by destroying it or listening to it. And if they don't we should get that logger error telling us that there is a faulty implementation (it shouldn't crash the entire server of course).

And if they don't we should get that logger error telling us that there is a faulty implementation (it shouldn't crash the entire server of course).

Fair enough.

And if they don't we should get that logger error telling us that there is a faulty implementation (it shouldn't crash the entire server of course).

Fair enough.

Test 1 is just a timeout issue and we might want to have a timeout mechanism somewhere to not get stuck.

Check, let's create an issue.

The reason that an error gets thrown in the case of the chained converter and not in the single case is because we happen to use something that is more robust than is needed in the creation of the original stream.

Wouldn't we always want to be robust then?
This is for me the main reason why I created the issue.
We both agree that the problem is with the convertor; but crashing the entire server as a consequence is strong.
By making the stream more robust, we can avoid that it becomes a #314 problem.
#314 is the very last resort: the server is a about to crash; the only reason it does not is because we remove the symptom.
Whereas by being robust, we can encapsulate the cause and kill the request instead.

Test 1 is just a timeout issue and we might want to have a timeout mechanism somewhere to not get stuck.

Check, let's create an issue.

The reason that an error gets thrown in the case of the chained converter and not in the single case is because we happen to use something that is more robust than is needed in the creation of the original stream.

Wouldn't we always want to be robust then?
This is for me the main reason why I created the issue.
We both agree that the problem is with the convertor; but crashing the entire server as a consequence is strong.
By making the stream more robust, we can avoid that it becomes a #314 problem.
#314 is the very last resort: the server is a about to crash; the only reason it does not is because we remove the symptom.
Whereas by being robust, we can encapsulate the cause and kill the request instead.

If we want to do this somewhere we should do it in the guardStream function as that is where all our streams pass by.

Yes; given the importance of streams in the architecture, that would also be my suggestion.

The only way to do this that I can think of would be to catch all addEventListener calls and wrap the callback functions in a try/catch (and somehow make sure we don't break the removeListener since we changed the associated functions).

Currently, guardStream changes the stream in-place. What we could do is instead return a passhtrough that wraps around the source stream, which then indeed adds wrapped callback functions to the source stream.

This would also not catch the 3rd test since callbacks are also not supposed to return promises as that one is doing.

We can also account for that in the wrapping function, by inspecting the return value and hooking up a catch handler if it's a promise.

Question is whether we want all of this. But when developing a convertor, it was so weird that the server could just crash in its entirety. I think it should be our goal to only make requests crash.

If we want to do this somewhere we should do it in the guardStream function as that is where all our streams pass by.

Yes; given the importance of streams in the architecture, that would also be my suggestion.

The only way to do this that I can think of would be to catch all addEventListener calls and wrap the callback functions in a try/catch (and somehow make sure we don't break the removeListener since we changed the associated functions).

Currently, guardStream changes the stream in-place. What we could do is instead return a passhtrough that wraps around the source stream, which then indeed adds wrapped callback functions to the source stream.

This would also not catch the 3rd test since callbacks are also not supposed to return promises as that one is doing.

We can also account for that in the wrapping function, by inspecting the return value and hooking up a catch handler if it's a promise.

Question is whether we want all of this. But when developing a convertor, it was so weird that the server could just crash in its entirety. I think it should be our goal to only make requests crash.

Question is whether we want all of this. But when developing a convertor, it was so weird that the server could just crash in its entirety. I think it should be our goal to only make requests crash.

The other solution there would be to start running requests all in their own process of course but that brings all the other problems

Question is whether we want all of this. But when developing a convertor, it was so weird that the server could just crash in its entirety. I think it should be our goal to only make requests crash.

The other solution there would be to start running requests all in their own process of course but that brings all the other problems

Another option is .addGuardedListener and .removeGuardedListener methods (and/or an guardListener).

Another option is .addGuardedListener and .removeGuardedListener methods (and/or an guardListener).

Another option is .addGuardedListener and .removeGuardedListener methods (and/or an guardCallback).

Not sure what you mean with this. Adding these functions to a guarded stream? But then we also depend on other libraries using these instead, just like now we depend on them not throwing errors in the callbacks.

Actually, having a guardCallback(eventEmitter, callback) would be a nice generic mechanism, to have a destination where errors should land.

If I understand correctly I think this would be similar to my suggestion above on how to guard the event listeners of a guarded stream. Which is definitely doable but comes with several things that need to be take into account.

Another option is .addGuardedListener and .removeGuardedListener methods (and/or an guardCallback).

Not sure what you mean with this. Adding these functions to a guarded stream? But then we also depend on other libraries using these instead, just like now we depend on them not throwing errors in the callbacks.

Actually, having a guardCallback(eventEmitter, callback) would be a nice generic mechanism, to have a destination where errors should land.

If I understand correctly I think this would be similar to my suggestion above on how to guard the event listeners of a guarded stream. Which is definitely doable but comes with several things that need to be take into account.

Not sure what you mean with this. Adding these functions to a guarded stream? But then we also depend on other libraries using these instead, just like now we depend on them not throwing errors in the callbacks.

Indeed; then the behavior is explicit, but you can at least opt to use it.

Not sure what you mean with this. Adding these functions to a guarded stream? But then we also depend on other libraries using these instead, just like now we depend on them not throwing errors in the callbacks.

Indeed; then the behavior is explicit, but you can at least opt to use it.

Isn't the behaviour we're discussing here quite similar to the (deprecated) domains?

Isn't the behaviour we're discussing here quite similar to the (deprecated) domains?

Isn't the behaviour we're discussing here quite similar to the (deprecated) domains?

It's much more local; we just want to contain the behavior of streams.

Another example I just came across. With the solid-hue module, sending a PUT request with a syntactically invalid RDF body crashes the server. For instance $ curl -X PUT localhost:3000/home/lights -H "Content-Type: text/turtle" -d '{' will
2021-01-07T22:39:58.468Z [StreamUtil] warn: Piped stream errored with Expected entity but got eof on line 2.
events.js:292
      throw er; // Unhandled 'error' event
      ^

BadRequestHttpError: Expected entity but got eof on line 2.
    at /Users/ruben/Documents/UGent/Solid/solid-hue/node_modules/@solid/community-server/dist/storage/conversion/RdfToQuadConverter.js:35:73
    at PassThrough.<anonymous> (/Users/ruben/Documents/UGent/Solid/solid-hue/node_modules/@solid/community-server/dist/util/StreamUtil.js:41:40)
    at PassThrough.emit (events.js:327:22)
    at N3StreamParser.<anonymous> (/Users/ruben/Documents/UGent/Solid/solid-hue/node_modules/rdf-parse/lib/RdfParser.js:71:47)
    at N3StreamParser.emit (events.js:327:22)
    at N3Parser.<anonymous> (/Users/ruben/Documents/UGent/Solid/solid-hue/node_modules/n3/lib/N3StreamParser.js:39:21)
    at N3Parser._error (/Users/ruben/Documents/UGent/Solid/solid-hue/node_modules/n3/lib/N3Parser.js:931:10)
    at N3Parser._readEntity (/Users/ruben/Documents/UGent/Solid/solid-hue/node_modules/n3/lib/N3Parser.js:200:21)
    at N3Parser._readSubject (/Users/ruben/Documents/UGent/Solid/solid-hue/node_modules/n3/lib/N3Parser.js:272:35)
    at /Users/ruben/Documents/UGent/Solid/solid-hue/node_modules/n3/lib/N3Parser.js:1073:127
Emitted 'error' event on PassThrough instance at:
    at PassThrough.onerror (_stream_readable.js:753:14)
    at PassThrough.emit (events.js:315:20)
    at emitErrorNT (internal/streams/destroy.js:100:8)
    at emitErrorCloseNT (internal/streams/destroy.js:68:3)
    at processTicksAndRejections (internal/process/task_queues.js:84:21) {
  statusCode: 400
}

Note the total lack of error context here.
I can prevent the crash by adding a representation.data.on('error', console.error), and representation.data.on('error', e => data.emit('error', e)) even nicely terminates the request.
However, does this mean that every convertor needs to listen to representation.data's errors? By that logic, more converter in the server might have a bug (although pipeSafely perhaps prevents this).
So I do think I disagree with your earlier

I guess this is the part I disagree with 😄. I do think that something that implements RepresentationConverter has the responsibility of handling that dat

Ways out:

Can we ensure that whoever passes a representation to a converter, attaches an error handler to the representation?

That way, we don't have to repeat that code, and we can be sure it is consistent.

Can GuardedStreams always pipe safely?
…

Isn't the behaviour we're discussing here quite similar to the (deprecated) domains?

It's much more local; we just want to contain the behavior of streams.

Another example I just came across. With the solid-hue module, sending a PUT request with a syntactically invalid RDF body crashes the server. For instance $ curl -X PUT localhost:3000/home/lights -H "Content-Type: text/turtle" -d '{' will
2021-01-07T22:39:58.468Z [StreamUtil] warn: Piped stream errored with Expected entity but got eof on line 2.
events.js:292
      throw er; // Unhandled 'error' event
      ^

BadRequestHttpError: Expected entity but got eof on line 2.
    at /Users/ruben/Documents/UGent/Solid/solid-hue/node_modules/@solid/community-server/dist/storage/conversion/RdfToQuadConverter.js:35:73
    at PassThrough.<anonymous> (/Users/ruben/Documents/UGent/Solid/solid-hue/node_modules/@solid/community-server/dist/util/StreamUtil.js:41:40)
    at PassThrough.emit (events.js:327:22)
    at N3StreamParser.<anonymous> (/Users/ruben/Documents/UGent/Solid/solid-hue/node_modules/rdf-parse/lib/RdfParser.js:71:47)
    at N3StreamParser.emit (events.js:327:22)
    at N3Parser.<anonymous> (/Users/ruben/Documents/UGent/Solid/solid-hue/node_modules/n3/lib/N3StreamParser.js:39:21)
    at N3Parser._error (/Users/ruben/Documents/UGent/Solid/solid-hue/node_modules/n3/lib/N3Parser.js:931:10)
    at N3Parser._readEntity (/Users/ruben/Documents/UGent/Solid/solid-hue/node_modules/n3/lib/N3Parser.js:200:21)
    at N3Parser._readSubject (/Users/ruben/Documents/UGent/Solid/solid-hue/node_modules/n3/lib/N3Parser.js:272:35)
    at /Users/ruben/Documents/UGent/Solid/solid-hue/node_modules/n3/lib/N3Parser.js:1073:127
Emitted 'error' event on PassThrough instance at:
    at PassThrough.onerror (_stream_readable.js:753:14)
    at PassThrough.emit (events.js:315:20)
    at emitErrorNT (internal/streams/destroy.js:100:8)
    at emitErrorCloseNT (internal/streams/destroy.js:68:3)
    at processTicksAndRejections (internal/process/task_queues.js:84:21) {
  statusCode: 400
}

Note the total lack of error context here.
I can prevent the crash by adding a representation.data.on('error', console.error), and representation.data.on('error', e => data.emit('error', e)) even nicely terminates the request.
However, does this mean that every convertor needs to listen to representation.data's errors? By that logic, more converter in the server might have a bug (although pipeSafely perhaps prevents this).
So I do think I disagree with your earlier

I guess this is the part I disagree with 😄. I do think that something that implements RepresentationConverter has the responsibility of handling that dat

Ways out:

Can we ensure that whoever passes a representation to a converter, attaches an error handler to the representation?

That way, we don't have to repeat that code, and we can be sure it is consistent.

Can GuardedStreams always pipe safely?
…

I am starting to think that I should write this code and this code as a Transform stream instead, and then pipeSafely into it. Thoughts?
Is that the pattern all converters should be using?

I can prevent the crash by adding a representation.data.on('error', console.error), and representation.data.on('error', e => data.emit('error', e)) even nicely terminates the request.

That is weird, because yes the entire point of the guarded stream is that the server should not crash if it emits an error. A guarded stream is a normal stream with an automatic error listener attached to it (which logs an error) so that error should not be uncaught. So definitely need to investigate what's going wrong there.
You should still listen to errors on the guarded stream though. Guarded just means (or should mean at least) that it's not going to crash the server if the stream emits an error, and that it will emit the error again later if an actual error listener gets attached. The original issue there was that error were being emitted while we were waiting for an other async operation to end. So the expectation is still that an error listener needs to be attached at some point for correct behaviour because that's an inherent feature of Node.js streams.
All our converters do listen to the error events though (because we use external libraries that do).

I can prevent the crash by adding a representation.data.on('error', console.error), and representation.data.on('error', e => data.emit('error', e)) even nicely terminates the request.

That is weird, because yes the entire point of the guarded stream is that the server should not crash if it emits an error. A guarded stream is a normal stream with an automatic error listener attached to it (which logs an error) so that error should not be uncaught. So definitely need to investigate what's going wrong there.
You should still listen to errors on the guarded stream though. Guarded just means (or should mean at least) that it's not going to crash the server if the stream emits an error, and that it will emit the error again later if an actual error listener gets attached. The original issue there was that error were being emitted while we were waiting for an other async operation to end. So the expectation is still that an error listener needs to be attached at some point for correct behaviour because that's an inherent feature of Node.js streams.
All our converters do listen to the error events though (because we use external libraries that do).

I am starting to think that I should write this code and this code as a Transform stream instead, and then pipeSafely into it. Thoughts?

I actually looked at what happens in those classes, and my first guess (but I didn't test yet) is that the crash is not related to the stream but due to the fact that the JSON.parse is going to crash in an async callback (the setImmediate) without having a try/catch around the parse. But then I can't explain why this wouldn't crash if you attach an error listener to the data stream so might have to look into it more. Using a Transform stream would not be bad practice though (and perhaps that one automatically emits an error instead if JSON.parse crashes).
In the second case the error might be because the end event is never emitted (since an error is emitted instead) but again might need more investigation.
But in general, yes you still need to listen to error events if you parse data from a stream. Guarding the stream just makes sure the error always gets emitted even if it got emitted before a listener was attached (buffering issue) and prevents the entire server from crashing (because the error is no longer uncaught, but this happening is a clear indication something is wrong in the code somewhere).

I am starting to think that I should write this code and this code as a Transform stream instead, and then pipeSafely into it. Thoughts?

I actually looked at what happens in those classes, and my first guess (but I didn't test yet) is that the crash is not related to the stream but due to the fact that the JSON.parse is going to crash in an async callback (the setImmediate) without having a try/catch around the parse. But then I can't explain why this wouldn't crash if you attach an error listener to the data stream so might have to look into it more. Using a Transform stream would not be bad practice though (and perhaps that one automatically emits an error instead if JSON.parse crashes).
In the second case the error might be because the end event is never emitted (since an error is emitted instead) but again might need more investigation.
But in general, yes you still need to listen to error events if you parse data from a stream. Guarding the stream just makes sure the error always gets emitted even if it got emitted before a listener was attached (buffering issue) and prevents the entire server from crashing (because the error is no longer uncaught, but this happening is a clear indication something is wrong in the code somewhere).

JSON.parse is going to crash in an async callback

JSON.parse is never invoked; the stream errors before we even get there.
I do understand why the behavior I describe occurs with regard to error handling (apart from the guarded stream apparently not working). I just find it a lot of responsibility on individual components and their implementers, so I'm looking to shift the burden.

JSON.parse is going to crash in an async callback

JSON.parse is never invoked; the stream errors before we even get there.
I do understand why the behavior I describe occurs with regard to error handling (apart from the guarded stream apparently not working). I just find it a lot of responsibility on individual components and their implementers, so I'm looking to shift the burden.

JSON.parse is never invoked; the stream errors before we even get there.

My mistake, then it's the await readableToString call that returns an error, but the result is the same: this is a function that throws an error that is not caught.

I do understand why the behavior I describe occurs with regard to error handling (apart from the guarded stream apparently not working). I just find it a lot of responsibility on individual components and their implementers, so I'm looking to shift the burden.

There is no way around having to attach an error listener to a stream if you're attaching a data/end listener though, changing that would require changing the kind of streams we're using I think.

JSON.parse is never invoked; the stream errors before we even get there.

My mistake, then it's the await readableToString call that returns an error, but the result is the same: this is a function that throws an error that is not caught.

I do understand why the behavior I describe occurs with regard to error handling (apart from the guarded stream apparently not working). I just find it a lot of responsibility on individual components and their implementers, so I'm looking to shift the burden.

There is no way around having to attach an error listener to a stream if you're attaching a data/end listener though, changing that would require changing the kind of streams we're using I think.

My mistake, then it's the await readableToString call that returns an error, but the result is the same: this is a function that throws an error that is not caught.

I think that's not correct, given that attaching an error handler fixes it.
I think the stream emits an error (before readableToString is able to reject) and that crashes the process.

There is no way around having to attach an error listener to a stream if you're attaching a data/end listener though, changing that would require changing the kind of streams we're using I think.

Check. However, I think we should:

Make that very clear in the contract (TSdoc is fine)
Guard the server against components that don't respect the contract (by accident, because it is so easy to do)

So let's investigate why the guard on representation.data is not working.
I will rewrite with Transform and report back (I am thinking of exposing a guardedTransform function to make doing the right thing easier).

My mistake, then it's the await readableToString call that returns an error, but the result is the same: this is a function that throws an error that is not caught.

I think that's not correct, given that attaching an error handler fixes it.
I think the stream emits an error (before readableToString is able to reject) and that crashes the process.

There is no way around having to attach an error listener to a stream if you're attaching a data/end listener though, changing that would require changing the kind of streams we're using I think.

Check. However, I think we should:

Make that very clear in the contract (TSdoc is fine)
Guard the server against components that don't respect the contract (by accident, because it is so easy to do)

So let's investigate why the guard on representation.data is not working.
I will rewrite with Transform and report back (I am thinking of exposing a guardedTransform function to make doing the right thing easier).

I just noticed readableToString takes a Readable as input and not a guarded stream so I wonder if perhaps the input stream is not being guarded somewhere (since it's not checked by the typings).

I just noticed readableToString takes a Readable as input and not a guarded stream so I wonder if perhaps the input stream is not being guarded somewhere (since it's not checked by the typings).

Yeah, and why doesn't readableToString attach an error handler? Seems to be its job.

Yeah, and why doesn't readableToString attach an error handler? Seems to be its job.

That it does do, specifically the library it uses does that: https://github.com/rubensworks/arrayify-stream.js/blob/master/index.js#L7

That it does do, specifically the library it uses does that: https://github.com/rubensworks/arrayify-stream.js/blob/master/index.js#L7

That it does do, specifically the library it uses does that: https://github.com/rubensworks/arrayify-stream.js/blob/master/index.js#L7

It might be too late; it's only attaching this inside of the new Promise, which executes async. https://github.com/rubensworks/arrayify-stream.js/issues/3

That it does do, specifically the library it uses does that: https://github.com/rubensworks/arrayify-stream.js/blob/master/index.js#L7

It might be too late; it's only attaching this inside of the new Promise, which executes async. https://github.com/rubensworks/arrayify-stream.js/issues/3

It might be too late; it's only attaching this inside of the new Promise, which executes async. rubensworks/arrayify-stream.js#3

This should not be an issue if the input is a guarded stream (since that's what it's supposed to prevent), so still would have to see if there's a problem with guarded streams or if this stream is not being guarded.

It might be too late; it's only attaching this inside of the new Promise, which executes async. rubensworks/arrayify-stream.js#3

This should not be an issue if the input is a guarded stream (since that's what it's supposed to prevent), so still would have to see if there's a problem with guarded streams or if this stream is not being guarded.

Agreed, but there's multiple protection mechanisms failing us here; we should get them all right.

Agreed, but there's multiple protection mechanisms failing us here; we should get them all right.

Agreed, but there's multiple protection mechanisms failing us here; we should get them all right.

But even if all the failsafes are working this code would still crash since an error is being thrown in a setImmediate call. So we have to make sure we can pinpoint what exactly the problem is.

Agreed, but there's multiple protection mechanisms failing us here; we should get them all right.

But even if all the failsafes are working this code would still crash since an error is being thrown in a setImmediate call. So we have to make sure we can pinpoint what exactly the problem is.

I added a unit test which replicates this behaviour so I can have a look

I added a unit test which replicates this behaviour so I can have a look

But even if all the failsafes are working this code would still crash since an error is being thrown in a setImmediate call.

Yeah, the code is definitely wrong. My reasoning was: there's nothing in my code that can throw, so I can get away with this. I understand now that this wasn't the right reasoning; but we still need to guard against devs attempting the same. I'm fine with the request errorring (preferred), or hanging (if we must), just not the server crashing. But for setImmediate, it's indeed a #314 issue.

But even if all the failsafes are working this code would still crash since an error is being thrown in a setImmediate call.

Yeah, the code is definitely wrong. My reasoning was: there's nothing in my code that can throw, so I can get away with this. I understand now that this wasn't the right reasoning; but we still need to guard against devs attempting the same. I'm fine with the request errorring (preferred), or hanging (if we must), just not the server crashing. But for setImmediate, it's indeed a #314 issue.

Tracking in issue https://github.com/solid/community-server/issues/489 (since this is a PR)

Tracking in issue https://github.com/solid/community-server/issues/489 (since this is a PR)

That it does do, specifically the library it uses does that: https://github.com/rubensworks/arrayify-stream.js/blob/master/index.js#L7

It might be too late; it's only attaching this inside of the new Promise, which executes async. rubensworks/arrayify-stream.js#3

That was not correct, the library executes fine: https://github.com/rubensworks/arrayify-stream.js/pull/4
So the bug might be in readableToString

That it does do, specifically the library it uses does that: https://github.com/rubensworks/arrayify-stream.js/blob/master/index.js#L7

It might be too late; it's only attaching this inside of the new Promise, which executes async. rubensworks/arrayify-stream.js#3

That was not correct, the library executes fine: https://github.com/rubensworks/arrayify-stream.js/pull/4
So the bug might be in readableToString

That was not correct, the library executes fine: rubensworks/arrayify-stream.js#4
So the bug might be in readableToString

There is definitely a bug somewhere, already did some research into it but still not sure where. Will spend some more time on it. There is an error listener attached to the stream when it enters the scheduleCrash call, but for some reason it still crashes later on.

That was not correct, the library executes fine: rubensworks/arrayify-stream.js#4
So the bug might be in readableToString

There is definitely a bug somewhere, already did some research into it but still not sure where. Will spend some more time on it. There is an error listener attached to the stream when it enters the scheduleCrash call, but for some reason it still crashes later on.

I'm pretty sure I found the problem (or at least a problem): the problem is that an error listener gets removed from a stream, after which an error gets emitted before the guarded stream had time to attach the guarding listener again.
For where exactly this happens I'm going to link to a part of the pipe functionality of Node.js here. This error listener gets attached to the destination when piping a stream. This means that our guard listener would be removed (since another error listener gets attached). But in the error callback there the listener gets removed (L762) and immediately after that it is checked if there are any error listeners remaining. Since this is in the same sync block there has not yet been an addListener event which our guarded stream is waiting for to add the safety mechanism back.
If I change the guarded stream behaviour to always keep the backup error listener attached (but not let it actually do anything unless it's the only listener) this crash no longer occurs so that would be a solution.
In the end I managed to create a very small test to reproduce this behaviour: just a chain converter with an RdfToQuadConverter and the CrashingConverter with an empty scheduleCrash function (so nothing extra was even needed there) and giving as input an invalid turtle string, which does seem to indicate that our testing of invalid input is inadequate (unless I'm totally wrong above what I said above of course).

I'm pretty sure I found the problem (or at least a problem): the problem is that an error listener gets removed from a stream, after which an error gets emitted before the guarded stream had time to attach the guarding listener again.
For where exactly this happens I'm going to link to a part of the pipe functionality of Node.js here. This error listener gets attached to the destination when piping a stream. This means that our guard listener would be removed (since another error listener gets attached). But in the error callback there the listener gets removed (L762) and immediately after that it is checked if there are any error listeners remaining. Since this is in the same sync block there has not yet been an addListener event which our guarded stream is waiting for to add the safety mechanism back.
If I change the guarded stream behaviour to always keep the backup error listener attached (but not let it actually do anything unless it's the only listener) this crash no longer occurs so that would be a solution.
In the end I managed to create a very small test to reproduce this behaviour: just a chain converter with an RdfToQuadConverter and the CrashingConverter with an empty scheduleCrash function (so nothing extra was even needed there) and giving as input an invalid turtle string, which does seem to indicate that our testing of invalid input is inadequate (unless I'm totally wrong above what I said above of course).

If I change the guarded stream behaviour to always keep the backup error listener attached

Perfect. Good sleuthing!

If I change the guarded stream behaviour to always keep the backup error listener attached

Perfect. Good sleuthing!


Updating links

Several links needed to be updated since RSPM and RSC migrated from Bookdown to MkSocs

Thanks for fixing these!


missing license/notice files in final jars

get api jar from maven, ie https://repo1.maven.org/maven2/jakarta/inject/jakarta.inject-api/1.0/jakarta.inject-api-1.0.jar
check that some license and notice file is included in the jar, preferably under META-INF folder

=> no license nor notice file found. These files are required to be present in the main distribution jar as well as source jar, see https://www.eclipse.org/projects/handbook/#legaldoc-distribution for details

Resolved vi #10


Feature/issue 1505 update questionnaire for two regions

Updates needed to MB questionnaire.

Q. 6. housing (NOTE: please change to both MB and BC):

[ ] answers should read (in this order)-

I want to rent a place to live
I already rent a place to live
I want to buy a home
I already own a home

Q. 7. current level of English:

[ ] Remove Canadian Language Benchmark mention and levels

Q. 10 work

[ ] add "Select all that apply"

[ ] remove "None of the above"

@rasmus-storjohann-PG questionnaire should read exactly as is in the first tab here:
https://docs.google.com/spreadsheets/d/1gn4KH_qlVcosFIDiYENVw4uGMWpcvrJwy4zU9gMpPXE/edit?usp=sharing
Specific changes to make:
Q. 3. When I go to the questionnaire, expected behavior is there is a Q3. Instead now, it skips from Q2 to Q4. This question is expected to be the one about current immigration status, instead nothing is there and that question is currently Q9.

[ ] make Q3 immigration status Q

Q. 6. housing (NOTE: please change to both MB and BC):

[ ] Change order of questions and content. Answers should read (in this order)-
I want to rent a place to live
I already rent a place to live
I want to buy a home
I already own a home

Q. 7. current level of English:

[ ] Remove Canadian Language Benchmark mention and levels. Should only read None, Beginner, Intermediate, Fluent.

Q. 10 work

[ ] add "Select all that apply"
[ ] remove "None of the above"

Q. 11 Health care

[ ] Remove BC-specific "Prescription medication" and "Eye doctors"

@deniza-pg

Issues listed above are fixed as part of #1505


feat: sanitize broker responses

This PR tries to address a few warnings that LGTM found in our codebase, the primary one being that we didn't sanitize the data that was passing into express' Response.send() calls.

before calling expresss.Response.send(), filter the data through escapeHtml()
remove a redundant regex char in the log matching code
replace an inefficient regex when extracting the job id from /api/jobs/${id}/log
try to avoid some gotchas in matching a gist.github.com URL

Also, while on the subject... any opinions on whether it would be useful to add LGTM to the Actions for sniffing PRs?

nit: Is it worth adding a test for the HTML escaping, or is that too specific?

I think it's reasonable. Added in 2322c17a6a1139c79619ea6699307b9e8ffb58f5


better strands navigation

[x] order alphabetically
[x] pagination
[x] subject filter
[x] year filter
[x] pin strand
[ ] search

search won't be implemented at this time. created issue #39 to track this feature request


AccessToken vs RefreshToken

@tylerlong How to identify which token has expired.
What is the different response in case of Expiry of AccessToken and RefreshToken?

Refresh token is not used unless you are doing token refresh.
So if you are making normal API calls and get token issue it is about access token.
This is not a Java SDK question, so I will close it.


What's the best way to use locally?

If I see the area chart, for example, and I want to get a version of that going locally, what's the best way to do that? The code sample shows up like this:

But it would be nice to also show the imports. It looks like it has tailwind css classes, too. Is that required? Sorry if I've missed part of the docs. Would love a "Getting Started" or "Installing" section with a template repo.

Hey @mhkeller :).  This is an area to improve for sure.  It's challenging as I want to examples to focus on the concepts, but it's also painful to have to figure out imports, data, etc.  That's one reason I added the view data button to make it a little easier to understand.
With that said, each page does have a Page Source button at the top, and a conveient Copy button to grab the full page of source.

Page Source
Dialog

If you are viewing a component docs, there is also a Source button, which is nice if you just want to the implementation

Source
Dialog

I 100% need to add a "Getting Started" section and improve/streamline that process.  I also want a REPL (either embedded or quick open to Svelte REPL, SvelteLab, Stackblitz) but haven't had the time to integrate it (and was met with some challenges a few years ago when I tried, but I think things are much better now).

@techniq I'm up for taking a stab at some getting started docs. My assumption you want to recommend installation via npm install as opposed to copying code into a new project. One issue I think is that when looking at the Layerchart example source, it shows importing components from $lib/components, but when you npm install, the import is different. Also, there is the tailwind config that needs to happen. None of it is supper crazy, but likely would help get people started more smoothly.

I am all down for helping with the docs as well.
I am currently testing the libs and documenting any hickups I come across, I am totally down on making a few PRs with updates on the docs.

@mattlangeman That would be awesome!.  Yeah, I agree having to understand and jump from...
import Chart from '$lib/components/Chart.svelte';
import Area from '$lib/components/Area.svelte';
import Axis from '$lib/components/Axis.svelte';
...

to...
import { Chart, Area, Axis, ... } from 'layerchart'

is beyond ideal.  I have seen docs like Material UI which allow you to jump between concise and expand to full example...

I would love to have the best of both words going forward, but know it would be a descent lift.  I like keeping the authoring experience as streamlined as possible.
I'm wondering if we could enhance the codePreview Svelte preprocessor to identify these components and add them to the top.  Even a naive of find...
<SomeComponent>
  <AnotherComponent />
</SomeComponent>

and append the following to the top...
import { SomeComponent, AnotherComponent } from 'layerchart';

might be a good start, although it would definitely not be perfect.
I would love to see how other libraries handle this (most I've looked at are more explicit with the examples / sources, even having multiple copies.  That makes me nervous based on the current volume and future maintainability.

@percybolmer We have a discord server that we've been chatting through some of this if you want to join.
I really should add that to the README :)

I agree with making the authoring experience as streamlined as possible and not having to keep two copies of example code updated. I like the idea of trying to automatically add the import at the top of the code preview.

Here's the main part of where that code is in case it's helpful. i think i took the first version from maybe a very early version of the svelte REPL circa 2019. https://github.com/mhkeller/layercake/blob/59218331b184275fc6be34030a4125f399979281/src/routes/_site-components/DownloadBtn.svelte#L5

Honestly I get so confused by the discord UI I guess that makes me a dinosaur!

You are not alone! I struggle with all of the non-threaded chat group apps.

@mhkeller Ha, no worries.  yeah, we could do a simple regex replace, especially for the full page source since it's a simple string in each example's +page.ts.  That would improve the $lib/components => layerchart mismatch (regex would be a little tricky being multi-line and needing to handle default and named exports for the same file...
import Chart, { Svg } from '$lib/components/Chart.svelte';
import Area from '$lib/components/Area.svelte';
import Axis from '$lib/components/Axis.svelte';

to
import { Chart, Svg, Area, Axis } from 'layerchart';

I think having a starter project would be nice as well, similar to npm create svelte, i.e. npm create layerchart (or svelte-ux).  There is also svelte-add, but I haven't used it a bunch personally.
I  know there are a few things being mentioned:

Improve getting started, both with better (i.e. existing) docs, and possibly a quick way via CLI (npm create layerchart) or download button
Improve the doc examples, with easier way to copy examples
Possibly an interactive REPL or way to quickly experiment (using WebContainers or embedded REPL, or links to Svelte REPL, SvelteLab, StackBlitz, etc).

Initial getting started docs added by @mattlangeman

Some progress to report.  We've recently made LayerChart and Svelte UX monorepos using pnpm workspaces, and added npm create svelte-ux@latest to the Svelte UX repo.  Currently there is just a minimal and starter templates, but I plan to add a starter template that includes LayerChart.  Additional, the starter template is currently available to open in SvelteLab, StackBlitz, or Codesandbox using the badges in the README.

With that said, I'm considering adding npm create layerchart@latest as well to setup the project with LayerChart only, and have a few templates (minimal, starter, and possibly with Svelte UX).  Since they are both separate, but companion projects, it gets a little tricky where to draw a line, but I think this will be very helpful for both getting started quickly, testing ideas, diagnosing issues, etc.
Anyways, the first steps have been made, and we can iterate from here.

All imports on the docs/examples now use layerchart alias to improve copy/paste experience.  Plan to continue adding improvements to make it easy to open individual examples in a REPL/StackBlitz/etc


HG util: add best-effort C++ compatibility (fix #449)

Seems like this is the best we can do if we want to use stdatomic.h, and as pointed out in http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0943r1.html android uses it -- so we are not too far out on a limb (plus Mercury itself is all in C anyway).  Unfortunate that C/C++ conflict happened though...


Psych 0.7 ghost editor

Can you add a option for the Psych 0,7 ghost's cause it is hard to know where to put the player and opponent postitions when making a character

thanks for the suggestion! this will be added in 0.7.2

thanks!


Class based Optimize solvers.

@stsievert, so one can see the CI.

This is looking pretty good (and way better than my implementation, objopt). It looks like the interface I'd like is possible:
class L2Loss(Function):
    def __init__(self, A, y):
        self.A = A
        self.y = y

    def func(x):
        return LA.norm(self.A@x - self.y)**2

    def grad(x):
        return 2 * self.A.T @ (self.A@x - self.y)

if __name__ == "__main__":
    n, d = 100, 10
    A = np.random.randn(n, d)
    x_star = np.random.randn(d)
    y = np.sign(A @ x)

    loss = L2Loss(A, y)
    opt = Optimizer(loss)

    for iteration in range(100):
        opt.step()
        # use opt to compute other stats
    # use opt to calculate more stats

BTW, if I miss a message it's because I'm a bit overwhelmed with features to implement, so may need reminding.

I'm having Git difficulties, and want to propose a change. We can still support the old API with this function:
def minimize(func, x0, *args_, args=(), method=None, jac=None, hess=None, **kwargs):
    solve_kwargs = {key: kwargs.pop(key) for key in ['maxiter', 'callback',
                                                     'disp', 'maxfun']
                    if key in kwargs}
    fn = Function(func, args=args, grad=jac, hess=hess)
    fn.x = x0
    opts = [opt for opt in sys.modules[__name__]]
    viable_opts = [opt for opt in opts
                   if opt.__name__.lower() == method.lower().replace('-', '')]
    if len(viable_opts) > 1:
        raise ValueError('More than 1 viable optimizer was found for '
                         'method=' + str(method))
    opt = viable_opts[0](fn, *args_, **kwargs)
    return opt.solve(**solve_kwargs)

...though it's untested.